[Proposal]: New hook data structure

[ekryski]

I think we need a slight restructure of the hook object that provides a bit firmer “contract” for what can be returned/modified and also helps people avoid common pitfalls. Overall the current structure has worked very well but after a bunch of real-world usage and feedback from the community I think we could improve things.

Problem

Some issues that I’ve noticed:

• hook.params can be a bit of a dumping ground and can be harder to reason as to what is in params, especially as an app grows or hook chains become complex.
• putting keys onto hook.params that are required for things to work properly (ie. authenticated, provider) is sketchy and easy to accidentally overwrite causing much problems that can be catastrophic and hard to debug
• passing params from client to server that are not part of the query or data is pretty common place and currently clunky
• it's easy to accidentally query for all things when doing a find by forgetting to wrap your query

const query = { name: 'John' };

// the incorrect way.
// easy to mess up, returns everything (especially if pagination is off)
app.service('users').find(query)

// the correct way
app.service('users').find({ query })

• it's far too easy to accidentally remove or patch all records for a given service by passing null as the id. This is really not good!

// this is super sketchy an empty object for the `id` field and/or
// explicit `params.removeAll = true` should be required
// (you can currently write your own hook to do this)
app.service('users').remove(null, params)

Proposal

Personally I’d like to see the hook object look more like this:

{
  id: 1,
  app,
  service: 'users',
  method: 'get',
  transport: 'rest',
  authenticated: true,
  headers: {},
  params: {},
  query: {},
  client: {},
  data: {},
  result: { // always return data even when not paginated you just don't have other keys
    data: [],
    errors: [{ data, error }], // for failed records in bulk changes
  }
}

I realize this is a VERY big breaking change that touches every pretty much every module but there are reasons for it:

• A bunch of the params that should be read-only live at the top level of the hook object to prevent them from being overridden.
• hook.client should exist for special client params (ip, mac address, os, app version, etc.)
• hook.params.provider should be renamed to transport to reflect our nomenclature in the docs and moved to the root of the hook and read-only.
• Always have results be returned as hook.results.data. This reduces boilerplate we need to write to check if a response is paginated in each hook.
• Moving query outside of hook.params.query would not only actually separate the query object from params but would I think cognitively help people separate them and also make it easier to reason from the client side what is being sent.

This would also change service calls so that they are always:

app.service('users').find(query, params)
app.service('users').get(id or query, params)
app.service('users').patch(id or query (cannot be null), data, params)
app.service('users').update(id or query, data, params)
app.service('users').create(data, params)
app.service('users').remove(id or query (cannot be null), params)

It would be quite a bit of work but I think would:

• help alleviate some confusion that people have with querying
• prevent people from patching/removing with null inadvertently and overwriting/removing all data for a service
• prevent querying for all records by accident by not wrapping the query
• help separate concerns and make it easier to reason about the data passed from client to server and vice-versa.
• allow use to better document where custom params live on the client and the server
• allow us to better/more clearly override pagination server side

Migration Path

Since this touches pretty much every module and would be a breaking change we'd need to be able to roll it out incrementally. In order to do this I think we can:

• Update the service adapter tests to a new major version
• Migrate service adapters one at a time to a new major version (hopefully with community support)
• Update common hooks to support the new structure
• Update transport libs and other plugins as new major versions
• Update docs
• Update generator

I think we can create a hook(s) that you can include at the app or service level that would be able to map a new hook format back to the legacy one in order to keep things backwards compatible and ease migration.

Obviously learning from our previous issues with the feathers-authentication 1.x release we'd likely want to do pre-releases for these until all the dependent pieces are ready and documented.

---

If we are to proceed, I think the next release (Crow) is the best time to do it as will have brought feathers-hooks into core feathers itself, and we may be looking to rename feathers-hooks-common to feathers-hooks.

Obviously this is a proposal and likely to change. I've talked with @daffl and @corymsmith about this at length over the last year but I would ❤️ some input from the community and the rest of the @feathersjs/core-team. Feel free to comment or simply give a 👍 or 👎 .

Related Issues

• Route arguments are not passed from client to server #304
• Separate "query" from other parameters in service calls #557
• Change remove-all syntax. feathersjs-ecosystem/feathers-mongoose#74

I'm sure there are more related issues as some of the problems highlighted in here have come up a quite few times in Github and Slack.

[eddyystop]

These are good ideas to document for the future.

We have permissions, improved filtering, Sequelize ORM handling, integrating feathers-authentication-management that we need to first design and code with our limited resources.

They might have a bigger benefit for Feathers without causing disruption.

As an aside, renaming feathers-hooks-common to feathers-hooks will cause a world of confusion.

[hubgit]

Always have results be returned as hook.results.data. This reduces boilerplate we need to write to check if a response is paginated in each hook.

This probably needs a separate issue of its own for discussion, to look at how existing hooks handle get/find data/results, but perhaps there could be a helper method on the hook that would apply a function to either a single item or an array/object of items as appropriate (e.g. hook.each(item => { item.updatedAt = Date.now() }).

This would make it easier for a single hook to be applied when handling single items (get, patch, etc) or multiple items (find, etc).

The proposal is a good one when just considering find results, still.

[eddyystop]

@hubgit The getItems and replaceItems hook utilities already exist to do what you suggest. See https://docs.feathersjs.com/api/hooks-common.html#util-getitems-replaceitems

[ekryski]

As an update we've run into some issues with the this context in feathers-hooks-common so an addition to this proposal is to not use this anywhere and instead rename hook -> context.

[marshallswain]

@ekryski We also need to account for hook.errors. We could normalize this to always be an array. That would help with situations like bulk inserts on create where there are several records that fail, as is the case with feathersjs-ecosystem/feathers-mongoose#199

[beeplin]

100% agree with the proposals, especially for the new query syntax.

[cdimitroulas]

all for the changes to hook.results to always have the data array! It would be great to not have to check that in our hooks 👍

[jakobrosenberg]

Much of this seems to be achievable with one or more hook. It would be a hack compared to making the changes to the service library, but it would be a start.

Normalizing the result.data through a hook would be hard though. If it's added to the apps hook, it would run after all the service hooks, which would be too late.

Would it be possible to add a universal after hook that runs before all service hooks instead of after?

[zsf3]

Currently I have done following using an app level after hook.

result: { // always return data even when not paginated you just don't have other keys
  data: [],
  errors: [{ data, error }], // for failed records in bulk changes
}

Although, it does not feel right (to me atleast) to check if it is not a find call:

if (context.result && context.method != 'find') {
    context.result = { data: context.result };
}