Hooks returning new context

[vonagam]

I think that returning new context object from a hook should be depreciated.

Upsides:

1. Clearer expectations of a hook - it just a function that mutates a context.
2. Clearer expectations of a context - it stays the same all the way for a call.
3. Less bugs, less code, clearer code, more performant code for users and feathers.
4. Hooks returning new object lose their context changes on a throw unlike mutating ones.
5. Migration from returning new object to mutating existing one should have no problems.

Downsides:

1. No backward compatibility for such hooks, need to migrate such hooks (even if this is easy).

Example: look at snippets in #932, they will produce unexpected behaviour if some hook down the chain replaces a context. (Even though those snippets are just illustrations, it is relevant since errors came from not right expectations, but if replacing hooks were depreciated there would be no problems besides unnecessary return statements).

Error right in the first snippet (same problem in others):

async (context, next) => {
  try {
    // ...

    // next() may return new context instance
    // but context variable not updated
    await next();
    
    // ...
    
    // will return initial passed context
    // possibly replacing new instance, losing everything
    return context;
  } catch(error) {
    // ...
  }
}

So you wouldn't be able to write const { result } = await next() if you want to write return context. You would need to write:

const { result } = context = await next();
// or if you replace context
const { result } = context = await next(context);

What are your thoughts? Are there any use cases where replacing context with new instance is easier than mutating existing one? Any justification to keep such feature besides backward compatibility?

[daffl]

I'm going back and forth about this as well. I kind of agree, on the other hand, having hooks be more functional and immutable would help prevent many subtle and confusing issues. More specifically, the problem is that a hook can modify the calling objects in ways you usually wouldn't expect which is especially difficult when other services are called. For example with the following:

const otherHook = context => {
  context.params.query.name = 'david';
}

const myHook = context => {
  context.params.query.name = 'nobody';
  await context.app.service('otherservice').find(context.params);
}

app.service('otherservice').hooks(otherHook);
app.service('myservice').hooks(myHook);

Used like this:

const params = { query: { name: 'somebody' } };

await app.service('myservice').find(params);

console.log(params.query.name);

I'd definitely expect console.log(params.query.name) to show 'somebody' but it will actually be 'david' set in a totally different part of the application two levels down. Usually these kinds of issues are easily prevented by making a copy of the query like this:

const otherHook = context => {
  const { query = {} } = context.params;

  context.params.query = {
    ...query,
    name: 'david'
  }
}

But it is a best practise you have to be aware of. Technically this isn't really a Feathers issue but more of JavaScript itself but it becomes more strange when used in the context of Feathers.

[vonagam]

I understand the issue regarding passed in arguments (it mentioned by you here) because a user may want and have right to own such objects.

But a context object itself is not passed in and not owned by a user, because it is created internally by feathers. And if there is a clear convention that a hook mutates passed in context, there should be no problem (especially since it is used like this in almost all hooks already).

Actually here you want to say to a user, that feathers owns this context and that you guarantee that it is the same for duration of a method call.

Hooks are like middleware for a function call. And if you look at usage and conventions of middleware in transport libraries that feathers use (express, socketio, koa) - they pass in objects for mutations and do not support replacing them.

Because of that i think there should be distinction of convention regarding changing context properties and context object itself.

[daffl]

I agree. And since the new hook system relies on koa-compose it probably makes sense to keep that behaviour. I honestly have no idea how often returning a completely new hook context is being used (I don't think I ever needed it) but as you pointed out, it wouldn't be too difficult to change.

There are still options to prevent the issues I was mentioning by e.g adding a cloneDeep hook that clones all parameters before a method call.

[vonagam]

Well, then i suppose that this issue can be closed for now. Can be reopened later if unexpected arguments for keeping replacing arrive.