-
-
Notifications
You must be signed in to change notification settings - Fork 796
Description
I think that returning new context object from a hook should be depreciated.
Upsides:
- Clearer expectations of a hook - it just a function that mutates a context.
- Clearer expectations of a context - it stays the same all the way for a call.
- Less bugs, less code, clearer code, more performant code for users and feathers.
- Hooks returning new object lose their context changes on a throw unlike mutating ones.
- Migration from returning new object to mutating existing one should have no problems.
Downsides:
- 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?