search

passport-next / passport

Simple, unobtrusive authentication for Node.js.
MIT License
36 stars 5 forks source link

Refactoring/Linting #19

Closed brettz9 closed 5 years ago

brettz9 commented 5 years ago

Is this a security patch?

No.

Checklist

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 55

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/sessionmanager.js 18 20 90.0%
<!-- Total: 147 149 98.66% -->
Totals Coverage Status
Change from base Build 54: -0.1%
Covered Lines: 343
Relevant Lines: 345
💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 55

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/sessionmanager.js 18 20 90.0%
<!-- Total: 147 149 98.66% -->
Totals Coverage Status
Change from base Build 54: -0.1%
Covered Lines: 343
Relevant Lines: 345
💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 55

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/sessionmanager.js 18 20 90.0%
<!-- Total: 147 149 98.66% -->
Totals Coverage Status
Change from base Build 54: -0.1%
Covered Lines: 343
Relevant Lines: 345
💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 78

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/sessionmanager.js 17 19 89.47%
<!-- Total: 157 159 98.74% -->
Totals Coverage Status
Change from base Build 71: -0.2%
Covered Lines: 336
Relevant Lines: 338
💛 - Coveralls
brettz9 commented 5 years ago

By the way, my own ESLint config (at https://github.com/brettz9/eslint-config-ash-nazg ) incorporates even more useful linting than airbnb if you were open to trying it (it expands on "eslint:recommended" and "standard" and several other plugins/configs), though for stylistic rules, the current style could be enforced.

adamhathcock commented 5 years ago

Definitely happy using more ES6 features.

brettz9 commented 5 years ago

Ok, I've pushed an additional commit which adds ash-nazg eslint config in place of airbnb. In order to demonstrate which airbnb rules we needed to add back (without overriding what I think are some useful stricter ash-nazg rules which airbnb disables), I am not currently extending from airbnb but have manually added its different (mostly) stylistic rules which the current code was expecting.

If you find this acceptable, I can remove the airbnb dev dependency.

If you prefer not to spell out the rules added back from airbnb manually and keep it as a dev dep and extend its config as well as ash nazg's, I can do that instead.

brettz9 commented 5 years ago

As far as the failing travis build, this is due to the linting step which fails for pre-8.3 Node (due to a dev dependency on eslint-plugin-node using object rest). Wondering if we could at least skip the linting for this version, if support is still needed?

I'm not clear on why the coverage is failing, as I haven't really added code or removed testing.

rwky commented 5 years ago

I've dropped support for node 6 in the master branch since it's EOL and pushed to this branch, travis is now happy.

Leave this with me and I'll review the changes, if @adamhathcock or any other @passport-next/developers want to review it then please go ahead the more the merrier.

@brettz9 thanks for the contribution! Much appreciated :)

brettz9 commented 5 years ago

Great, thanks @rwky ... I'm working on a PR now for a Promise-based API (currently serializeUser, deserializeUser, and transformAuthInfo). I'm also including an option to return values synchronously where the former two had not previously supported this (though it doesn't work with "pass" or "undefined" which would be ambiguous). I can remove that if that is not favored for some reason.

adamhathcock commented 5 years ago

In general, I'm scared of moving too far from upstream code. I guess the question is how much do we want to risk not getting upstream changes/fixes? I prefer Promises, obviously :)

brettz9 commented 5 years ago

Isn't that the point of this repo though, that the original author wasn't responding? Since there haven't been any changes of note since then, I could submit these changes as a PR... I'd think ES features would be welcomed...

adamhathcock commented 5 years ago

I'm not against it. I'm just making sure the decision is a measured one.

rwky commented 5 years ago

I'm up for promises, this repo is so far from the upstream repo that any fixes that do land upstream that I haven't already merged in would need to be manually applied assuming they're relevant to us.

If possible I'd rather have changes me semver minor instead of semver major i.e. new stuff doesn't break existing features. Promises are discussed here https://github.com/passport-next/passport/issues/7#issuecomment-421174501

adamhathcock commented 5 years ago

Let's start breaking things.

rwky commented 5 years ago

I've checked the changes and I like them! I do want to make a few changes myself I'll sort it all out this week and merge this.

rwky commented 5 years ago

Just letting everyone know I've not forgotten about this just taking longer for me to do what I want than expected.

brettz9 commented 5 years ago

No worries... Also, my apologies for not yet getting to the Promise PR which I still hope and plan to complete...

brettz9 commented 5 years ago

I've gotten out the Promises changes in #21, though I'll probably want to try them out a bit in my own code to get a better feel for how it works in the real world. However, I'm wondering if you have an ETA on your changes desired for this Refactoring PR?

rwky commented 5 years ago

I've just got back off holiday so have the usual billion emails everyone gets when they get back ;) though this should be doable this week, I've got the code ready locally just need to tweak/test some things.

rwky commented 5 years ago

@brettz9 I've added a new PR for the refactoring in #22 can you take a look? If it's all ok and gets merged in then we can look at promises based off the new PR (it shouldn't take much effort to rebase).