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

Bad Hook API Design: Hooks are inconsistent and impure functions #288

Closed
markmarijnissen opened this Issue Sep 10, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@markmarijnissen
Copy link

markmarijnissen commented Sep 10, 2016

Some hooks have inconsistent return values and are impure:

For example:
https://github.com/feathersjs/feathers-authentication/blob/master/src/hooks/restrict-to-owner.js

  • Inconsistent return value: When not given a provider, it returns the hook (synchronous), while otherwise it will return a promise.
  • Impure: It depends on this to be bound the the Service.

This makes hook composition harder than it needs to be:

  • I need to wrap the hook result in Promise.resolve() to convert all possible return types to a Promise
  • I need to call the hook using embeddedHook.call(this,hook)

In my case, I was writing a hook that would restrict to roles OR owner.

Suggestion:

  • Add hook.service to the hook object so you can avoid this
  • Consistent return types (a promise in all cases)
@daffl

This comment has been minimized.

Copy link
Member

daffl commented Sep 14, 2016

Yes, I think @ekryski agrees that at least all built in hooks should always return a Promise (although they do resolve asynchronously which might not always end up being the same as synchronous hooks).

I'm not sure about the this reference since it is in the nature of JavaScript that this can just be set and should therefore be treated like any other function parameter. We can add a hook.service in the next version of feathers-hooks to reference the same thing.

I'm still trying to understand the use case where it is necessary to mock and test the built-in authentication hooks since we are already testing them here (and if test cases are missing we should add them to our test suite). Ideally in an application setup you can just test if the actual service calls behave as expected.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Sep 14, 2016

Yup definitely agree. That's been bothering me for a while. Currently in the process of rewriting them to always return promises.

We can add a hook.service in the next version of feathers-hooks to reference the same thing.

I think that's a good idea. Using this is not explicit if you are inspecting the hook object and forgot to read or missed it in the docs.

I'm still trying to understand the use case where it is necessary to mock and test the built-in authentication hooks since we are already testing them here (and if test cases are missing we should add them to our test suite). Ideally in an application setup you can just test if the actual service calls behave as expected.

💯 . No need to write additional tests outside of the module. This one has a lot and if we are missing some or they could use cleanup we'll gladly take contributions.

@markmarijnissen

This comment has been minimized.

Copy link
Author

markmarijnissen commented Sep 14, 2016

Did I say anything about testing?

Anyway, there are many reasons to test the build in hooks:

  • When a custom hook uses (wraps, embeds) a build in hook.
  • When you write a hook that depends on earlier build in hooks
  • To verify security of the app (it is just testing correct configuration of the hook)

Granted, last two are more integration than unit tests, but still nice to have.

The 'this' comment is perhaps a matter of personal preference. I just don't like using "hook.call(service,arg)"

@markmarijnissen

This comment has been minimized.

Copy link
Author

markmarijnissen commented Sep 14, 2016

I wouldn't do both .service and this BTW, as I am afraid this will lead to a mixed uses (some use hook.service, some this), and that'll make the feathers api surface bigger and more complicated than needed

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 26, 2016

This is fixed in 0.8-beta.

@ekryski ekryski closed this Oct 26, 2016

@ekryski ekryski added this to the 0.8 milestone Oct 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.