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

Clarify that the authenticate hook is required. #368

Merged
merged 1 commit into from Dec 14, 2016

Conversation

Projects
None yet
3 participants
@marshallswain
Copy link
Member

marshallswain commented Dec 13, 2016

The "Hooks" heading wasn't enough to catch my attention that the authenticate hook is required. It took me a while to figure out why my JWT didn't have any entity data in it.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Dec 13, 2016

Before merging lets finish the discussion in feathersjs/authentication-local#4 (comment)

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Dec 14, 2016

I think this is probably a good call. I also want to remove the code climate shit that was added. It's making the repo look like shit because I have a couple TODOs in there and because the tests have "similar code".

:shipit:

@marshallswain marshallswain merged commit 4303f6f into master Dec 14, 2016

3 of 4 checks passed

codeclimate 13 new issues
Details
codeclimate/coverage 82.31%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@marshallswain marshallswain deleted the clarify-hooks branch Dec 14, 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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.