Refactoring/Linting

[brettz9]

• Refactoring: Use ES6 classes, destructuring; use forEach; declare
  variables in more nested scope
• Linting: Remove jshint
• Linting: Apply overrides to add mocha-specific config
• Linting: Add recommended fields to package.json
• Linting: Remove unused disable directives
• Git/npm/Linting: ignore var

Is this a security patch?

No.

Checklist

• I have read the CONTRIBUTING guidelines. make view-cov does not work, however, as I get the message, "The file /Users/brett/passport/reports/coverage/lcov-report/index.html does not exist."
• I have added test cases which verify the correct operation of this feature or patch. N/A (still passing)
• I have added documentation pertaining to this feature or patch. N/A: No new features
• The automated test suite ($ make test) executes successfully.
• The automated code linting ($ npm run-script lint) executes successfully.

[coveralls]

Pull Request Test Coverage Report for Build 55

• 147 of 149 (98.66%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 96.315%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/sessionmanager.js	18	20	90.0%

Totals
Change from base Build 54:	-0.1%
Covered Lines:	343
Relevant Lines:	345

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 55

• 147 of 149 (98.66%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 96.315%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/sessionmanager.js	18	20	90.0%

Totals
Change from base Build 54:	-0.1%
Covered Lines:	343
Relevant Lines:	345

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 55

• 147 of 149 (98.66%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 96.315%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/sessionmanager.js	18	20	90.0%

Totals
Change from base Build 54:	-0.1%
Covered Lines:	343
Relevant Lines:	345

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 78

• 157 of 159 (98.74%) changed or added relevant lines in 6 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.2%) to 96.296%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/sessionmanager.js	17	19	89.47%

Totals
Change from base Build 71:	-0.2%
Covered Lines:	336
Relevant Lines:	338

---

💛 - Coveralls

[brettz9]

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]

Definitely happy using more ES6 features.

[brettz9]

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]

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]

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]

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]

Looks mostly fine to me. Can't tell if it will break things but a major version bump seems sensible.

[adamhathcock]

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]

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]

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

[rwky]

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 #7 (comment)

[adamhathcock]

Let's start breaking things.

[rwky]

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]

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

[brettz9]

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

[brettz9]

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]

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]

@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).