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

Make params optional #59

Closed
bedeoverend opened this issue Aug 17, 2016 · 6 comments
Closed

Make params optional #59

bedeoverend opened this issue Aug 17, 2016 · 6 comments

Comments

@bedeoverend
Copy link

Some of the methods e.g. get require params be given (by using params.sequelize). This is fine if dealing with an incoming request or via app.service (I believe) but if using the service directly without going through app it'll throw TypeError: Cannot read property 'sequelize' of undefined

@daffl
Copy link
Member

daffl commented Aug 17, 2016

All Feathers service methods run through the normalizer mixin which should set params to an empty object if it doesn't exist.

You do have to make sure to get the service via app.service('servicename')instead of using the service instance directly.

@bedeoverend
Copy link
Author

@daffl is that expected behavior? I noticed quite a few other methods in other feathers hooks do a check for params (e.g. params && params.foo). I would expect the ability for it to still work using the service instance directly.

If this isn't expected behavior, or a concern for this plugin, then happy to close, otherwise also happy to do a PR that runs the checks.

@daffl
Copy link
Member

daffl commented Aug 17, 2016

You shouldn't use the service instance directly. You won't get hooks or real-time events or anything else that Feathers adds (see here).

The hook checks are probably a relic from pre-normalization times (I think that was < 1.3).

@bedeoverend
Copy link
Author

@daffl Ok, thanks for the information. Happy to close this

@daffl
Copy link
Member

daffl commented Aug 17, 2016

Thank you for bringing it up. We'll make sure to mention it in more spots in the documentation to make sure it is well known.

@bedeoverend
Copy link
Author

bedeoverend commented Aug 18, 2016

@daffl No worries, thanks for the quick responses. My interpretation was that it was best practice to get it via app.service, rather than a necessity. But having said that, and looking back over the docs, the interface of the methods does indicate params as a guaranteed parameter, rather than optional like the callback param: get(id, params[, callback]). As you say though, be great if there is more mention in the docs 😃

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

No branches or pull requests

2 participants