proposal: get on hyper services should return the "hyper response" shape, not the resource.

[TillaTheHun0]

We've had internal conversations about this, but I would like to track this as it's increasingly becoming a point of friction with hyper developers.

Background

Almost all hyper service apis return a common shape, I'll refer to as a "hyper response shape". This looks like:

interface HyperResponse {
  ok: boolean,
  msg?: string
  status?: number
}

If ok is false, msg and status may be provided for additional context. For example, if a document isn't found, getDocument will return a 404 status.

For adapters, we encourage the status to be some http status code (no point re-inventing the wheel), but that is just convention.

Otherwise, if ok === true, service api responses will include a field on the "hyper response" ie. data.queryDocuments and cache.listDocs will have a docs field and search.query will have a matches field. Effectively ok is discriminator field.

Similar to fetch, hyper-connect, our hyper REST client, will only throw if a 5xx status code is received from a request to a hyper REST api. So 4xx must be handled by consumer code, which is by design. A common pattern is something like:

const res = await hyper.data.query(...)

if (!res.ok) {
  // process the error, maybe even look at res.status
  throw new Error(res.msg)
}

// do something with res.docs

Since ok is a discriminator, this works great with TypeScript, as TS is smart enough to inform Intellisense which fields are available on res based on the value of ok.

Problem

The exception to this is the get apis for data and cache. When fetching a single resource, hyper service apis will return the resource if found, and a "hyper response" if not found. This divergence from the common shape results in unergonomic consumer code. For example:

const res = await hyper.data.get(...) // either an object or a hyper response with ok: false

/**
 * This check is no longer reliable, as it could overlap with the resource object returned
 * What if the resource itself has an `ok` field? What that field is false?
*/
if (!res.ok) {
  ....
}

// do something res directly, not res.doc

Checking status has similar issues. What if the resource itself has status field?

On top of this, TS doesn't catch this overlap, making get a potential source of esoteric bugs, if a resource happens to have a ok or status field.

Basically, get must be coded for differently than every other hyper service api, and also may introduce unintended bugs in consumer code.

Potential solutions

Make get on hyper service adapters return a "hyper response"

Instead of the resource, get for each service would return a field on a hyper response, ie. data.getDocument could instead return:

{
  ok: true,
  doc: {...}
}

This would mean get is handled just like any other hyper service response.

Pros

• Makes the api consistent

Cons

• Breaking change for consumers
• Breaking change for adapters
• Breaking change for core

Make the hyper app return a hyper response

We could make the hyper app, ie. hyper-app-opine take the response from core and wrap it in a hyper response ie.

const coreRes = core.data.getDocument(...)

res.json({ ok: true, doc: coreRes})

Pros

• Makes the api consistent for consumers
• adapters don't have to change

Cons

• Doesn't make api consistent in adapters impls
• Breaking change for consumers
• Is app really the appropriate place for this change?

Make hyper core return a hyper response

We could make the hyper core, take the response from the adapter and wrap it in a hyper response ie.

const adapterRes = dataAdapter.getDocument(...)

return { ok: true, doc: adapterRes }

Pros

• Makes the api consistent for consumers
• adapters don't have to change

Cons

• Doesn't make api consistent in adapters impls
• Breaking change for consumers
• Breaking change in core

[TillaTheHun0]

No matter what, this is a breaking change for consumers, which stinks. Idk how to get around that.

[twilson63]

Making it return a hyper response if fine with me, maybe the hyper sdk can help with the breaking change

[TillaTheHun0]

maybe the hyper sdk can help with the breaking change

Maybe adding an option to each of the get calls on each service on hyper-connect. Something like legacy: true that would unwrap the hyper response and just return the resource. legacy would default to true, then the breaking change would be defaulting to false, or removing the option, entirely.

This means folks will need to update their hyper-connect version still though. Hmm, maybe expose as a header on app-opine, like x-hyper-get-legacy: 1 and unwrap inside of the opine-app for each get endpoint. Default to 1 (true) , then eventually default to 0 or remove entirely. This would enable consumers to incrementally adopt and also not require updating hyper-connect version to prevent a breaking change. I like that option better.

[TillaTheHun0]

I like the approach I laid out in my last comment:

Add support for a x-hyper-legacy-get header on hyper-app-opine for get endpoints. Default it to enabled or 1. That will be be used to instruct core to follow legacy behavior and return the resource itself, or to return a hyper response for get calls. Then push a breaking change on hyper-connect that sets x-hyper-legacy-get to disabled or 0.

I'll need to add a migration guide in the docs for consuming via REST and via hyper-connect.

Eventually, we can default x-hyper-legacy-get to 0 on hyper-app-opine as a breaking change.

[twilson63]

Yep Sounds great!

…

Sent from my iPhone

On Nov 4, 2022, at 5:30 PM, Tyler Hall ***@***.***> wrote:  I like the approach I laid out in my last comment: Add support for a x-hyper-legacy-get header on hyper-app-opine for get endpoints. Default it to enabled or 1. That will be be used to instruct core to follow legacy behavior and return the resource itself, or to return a hyper response for get calls. Then push a breaking change on hyper-connect that sets x-hyper-legacy-get to disabled or 0. I'll need to add a migration guide in the docs for consuming via REST and via hyper-connect. Eventually, we can default x-hyper-legacy-get to 0 on hyper-app-opine as a breaking change. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

[TillaTheHun0]

I need to update docs at doc.hyper.io with directions on how to use X-HYPER-LEGACY-GET header and the directions on how to migrate.

I'll create separate issues to:

1. Make the default for legacyGet be false, which will still allow folks to opt-in to legacy explicitly (a breaking change on hyper)
2. Remove the legacyGet api (a breaking change on core)

[TillaTheHun0]

• Update docs (docs are here https://docs.hyper.io/legacy-get-migration-guide)
• Create followup issues
  • Tracking default isLegacyGetEnabled to false for Data and Cache api #550
  • Tracking remove isLegacyGetEnabled flag from Data and Cache service #551

[TillaTheHun0]

See #546

I'll have to update the ports to the new shape along with the adapters, in tandem, which I suppose is the point, when we decide to make the swap over the new shape as the default.