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

Turn .params, .props and .defaults into hooks #36

Closed
daffl opened this issue May 28, 2020 · 2 comments · Fixed by #37
Closed

Turn .params, .props and .defaults into hooks #36

daffl opened this issue May 28, 2020 · 2 comments · Fixed by #37

Comments

@daffl
Copy link
Member

daffl commented May 28, 2020

While working on #35 I realized that most of what the hook manager does for .params, .props and .defaults could really be done by hooks themselves like this:

import { hooks, middleware, params, props, defaults } from '@feathersjs/hooks';

const sayHello = async (firstName, lastName) => {
  return `Hello ${firstName} ${lastName}!`;
};

const helloWithHooks = hooks(sayHello, [
  params('firstName', 'lastName'),
  props({ someProperty: true }),
  defaults((context) => ({
    lastName: 'unknown'
  })),
  async (context, next) => {
    // Do things here
    await next();
  }
]);

I did a quick benchmark and it seems to be just as fast as the current setup. I find it more consistent than having a separate API.

@bertho-zero
Copy link
Collaborator

It can work like this but I see 2 drawbacks:

  • params, props and defaults are commonly used names, it can cause conflict with the user variable names
  • the hook manager assures us that what manages the params is always executed before the rest of the hooks

I wonder if the hooks attached to the class will still know the params that are defined at the method level?

@daffl
Copy link
Member Author

daffl commented May 29, 2020

Very good points! I will give it a try and see if it also works properly with classes and inheritance. For the first one, if I understand it right, I think you should be able to alias the import to the name you need:

import { hooks, middleware, params as hookParams } from '@feathersjs/hooks';

const sayHello = async (firstName, lastName) => {
  return `Hello ${firstName} ${lastName}!`;
};

const helloWithHooks = hooks(sayHello, [
  hookParams('firstName', 'lastName'),
  async (context, next) => {
    // Do things here
    await next();
  }
]);

Or just give it better names but I'm not sure what those would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants