Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hooks): Refactor .params, .props and .defaults into hooks #37

Merged
merged 4 commits into from
Jun 1, 2020

Conversation

daffl
Copy link
Member

@daffl daffl commented May 30, 2020

This pull request is another refactoring that turns params, props and defaults into setContext.params, setContext.properties and setContext.defaults hooks. Performance appears to be about the same (or better when you do not use those hooks) with a more simplified API and smaller code base (client side bundle size reduced by ~20%). It can be used like this:

const { hooks, setContext } = require('@feathersjs/hooks');

const sayHello = async name => {
  return `Hello ${name}!`;
};

const wrappedSayHello = hooks(sayHello, [
  // Sets `context.name`
  setContext.params('name'),
  // Sets `context.enabled
  setContext.properties({
    enabled: true
  }),
  // Sets default name (if not available)
  setContext.defaults(context => {
    return { name: 'David' }
  }),
  async (context, next) => {
    console.log(context.arguments);
    await next();
  }
]);

(async () => {
  console.log(await wrappedSayHello('David'));
})();

Closes #36
Closes #38

@daffl daffl merged commit 9b13b7d into master Jun 1, 2020
@daffl daffl deleted the hook-built-ins branch June 1, 2020 17:17
@bertho-zero
Copy link
Collaborator

I think this PR is a regression:

As I say here #36 (comment) the hook manager assures us that what manages the parameters is always executed before the rest of the hooks, I cannot know context.params in a hook attached to the class level. (https://runkit.com/embed/ewg0wrcqlj8o)
It is a potential source of error depending on the order in which we call params, props and defaults. defaults should be after params.
It is no longer possible to know the name of the params before executing the function, it is an info which can be useful.

@daffl
Copy link
Member Author

daffl commented Jun 4, 2020

Yeah, you're totally right and I ran into this problem literally right away 🤦 I like having it as a hook though since it's more consistent, less code to maintain and it can be faster. Should we bring back the options again then?

const { hooks, middleware } = require('@feathersjs/hooks');

const sayHello = async name => {
  return `Hello ${name}!`;
};

const wrappedSayHello = hooks(sayHello, middleware([
  async (context, next) => {
    console.log(context.name);
    await next();
  }
], {
  params: ['name'],
  defaults: context => ({ name: 'Dave', params: {} }),
  properties: { method: 'Something' }
}));

This would register a standard hook that always runs first and initializes those properties.

@bertho-zero
Copy link
Collaborator

I'm fine with the options, it's probably the best solution

bertho-zero added a commit that referenced this pull request Oct 24, 2020
daffl pushed a commit that referenced this pull request Nov 8, 2020
BREAKING CHANGES: This reverts back to the usage of v0.4.0 and before.
daffl pushed a commit that referenced this pull request Nov 12, 2020
BREAKING CHANGES: This reverts back to the usage of v0.4.0 and before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants