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

Promises #21

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

Promises #21

wants to merge 37 commits into from

Conversation

brettz9
Copy link
Member

@brettz9 brettz9 commented Jun 4, 2019

  • Breaking change: Stop supporting older behavior against layers of arity one or two
    (will now drop one with no done)
  • Breaking change: We now catch errors within layers so as to support more
    intuitive passing of errors (when synchronous)
  • Breaking change: No longer throws when login is missing a callback (as may use as Promise)
  • Breaking change: Only return from serializeUser, deserializeUser (and transformAuthInfo)
    when giving a synchronous serialized result or a Promise (do not do return done())
  • Fix: Avoid erring upon missing failure with callback and authenticate failure
  • Fix: Check for instance property (as in check earlier in the same method)
  • Enhancement: Promises for serializeUser, deserializeUser, transformAuthInfo
    (including allowing throwing new Error('pass') to pass); avoid need for done
  • Enhancement: Allow serializeUser or deserializeUser to return non-undefined
    value for sync behavior
  • Refactoring: Return Promise from req.logIn and from authenticate middleware's
    strategy.success
  • Refactoring: Avoid let in unneeded higher scope
  • Testing: Add tests with req.logIn and SessionStrategy returning without done
    (synchronously or with Promise)
  • Testing: Delete _passport object when simulating without initialize (as it is added there)
  • Testing: Rename strategy name to distinguish from fail method
  • Docs: Show example of a LocalStrategy using async/await
  • Docs: Avoid showing connect cookieParser with session;
    use current expressSession over connect.session
  • Linting: jsdoc
  • npm/Docs: Add jsdoc with script, along with static server and open-cli to open
  • npm: Add test-one script (to test a single file)
  • npm: Update devDeps

Fixes #7.

Checklist

  • I have read the CONTRIBUTING guidelines.
  • I have added test cases which verify the correct operation of this feature or patch.
  • I have added documentation pertaining to this feature or patch.
  • The automated test suite ($ make test) executes successfully.
  • The automated code linting ($ npm run-script lint) executes successfully.

@coveralls
Copy link

coveralls commented Jun 4, 2019

Pull Request Test Coverage Report for Build 79

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+3.5%) to 100.0%

Totals Coverage Status
Change from base Build 71: 3.5%
Covered Lines: 30
Relevant Lines: 30

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 79

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+3.5%) to 100.0%

Totals Coverage Status
Change from base Build 71: 3.5%
Covered Lines: 30
Relevant Lines: 30

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 4, 2019

Pull Request Test Coverage Report for Build 202

  • 166 of 166 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.4%) to 100.0%

Totals Coverage Status
Change from base Build 194: 2.4%
Covered Lines: 407
Relevant Lines: 407

💛 - Coveralls

@brettz9 brettz9 mentioned this pull request Jun 9, 2019
5 tasks
@rwky rwky self-requested a review June 10, 2019 13:32
@brettz9 brettz9 force-pushed the promises branch 2 times, most recently from 57411f2 to e440de7 Compare June 11, 2019 09:37
@rwky
Copy link

rwky commented Jun 25, 2019

@brettz9 can you rebase master onto this please?

@rwky rwky assigned rwky and brettz9 Jun 25, 2019
@brettz9 brettz9 force-pushed the promises branch 3 times, most recently from b06256c to 9de48b8 Compare June 26, 2019 00:11
@brettz9
Copy link
Member Author

brettz9 commented Jun 26, 2019

I've rebased, but want to first examine some failing linting errors (upon a further devDep update I added).

@brettz9 brettz9 force-pushed the promises branch 3 times, most recently from 5af3ab9 to 2b8613e Compare June 26, 2019 08:39
@brettz9
Copy link
Member Author

brettz9 commented Jun 26, 2019

Ok, I've addressed the linting errors now. I also fixed a few minor issues in templates.

Btw, would you mind our adding typescript as a devDep if for no other reason than to get the peer dep. warnings to disappear upon local installs and updates?

@brettz9 brettz9 force-pushed the promises branch 3 times, most recently from 7953bbf to af6aa76 Compare June 27, 2019 06:30
@rwky
Copy link

rwky commented Jun 27, 2019

I've nothing against typescript being a dev dep, feel free to add it in this PR.

@brettz9
Copy link
Member Author

brettz9 commented Jun 27, 2019

K, thanks, added...

@brettz9 brettz9 force-pushed the promises branch 2 times, most recently from d5c5407 to d47df15 Compare June 28, 2019 23:48
lib/http/request.js Outdated Show resolved Hide resolved
@rwky
Copy link

rwky commented Oct 8, 2019

I've updated passport local to work with skel.

@brettz9
Copy link
Member Author

brettz9 commented Nov 4, 2019

Just an FYI, I've gotten tied up in a subproject of a subproject (internationalization for a revamping of node-login--let me know if you might be interested in having something like the latter under passport-next's umbrella, btw).

My hope is that after settling this, I can be ready to test passport more fully.

@rwky
Copy link

rwky commented Nov 4, 2019

let me know if you might be interested in having something like the latter under passport-next's umbrella, btw

Depends what it does, do you have a github repo for it?

No worries it's only just November and I've been busy this month, there's no critical issues in the passport-next repos at the moment so there's no rush on anything.

@brettz9
Copy link
Member Author

brettz9 commented Nov 4, 2019

It's a fork of https://github.com/braitsch/node-login/ (with some changes thus far to improve security, update, lint/refactor, increase configurability/CLI control, UI testing/coverage, i18n, accessibility; but essentially it looks the same as that repo.

@rwky
Copy link

rwky commented Nov 7, 2019

Have you poked the upstream dev to see what's going on with maintenance?

@brettz9
Copy link
Member Author

brettz9 commented Nov 8, 2019

I had asked in one of my PRs. I've asked again, so we'll see.

… passport-next version once merged

- Linting (ESLint): Update per latest ash-nazg
- Maintenance: Add `.editorconfig`
- npm: Update devDeps (coveralls, linting, mocha, nyc)
@brettz9
Copy link
Member Author

brettz9 commented Apr 2, 2020

Hi @rwky,

Since the maintainer of node-login (the generic UI login tool) never replied to my PRs, I've gone ahead and forked the project to make a tentative release of my own, with a large number of improvements, putting out a release candidate today, getting ready for a 1.0 release: https://github.com/brettz9/nogin/

I've made it quite configurable by command line, so there should be little need for projects to directly modify the source. Besides other fixes and enhancements, I've added Cypress UI testing, a UI testing framework I really love as it has a code coverage plugin that makes for more real-world type testing, and we're at 100% UI (or CLI) coverage now, and passing tests in Firefox/Chrome (including even tests to check that email notices are received).

Tests should also run in Edge, but Cypress still has open (low priority) issues for supporting IE11 and Safari, so our tests wouldn't run there, but in theory the code will hopefully work in those browsers as well, as we are using a Babel/preset-env + core-js Rollup routine that should compile to a safe syntax (as well as eslint-plugin-compat to catch use of browser HTML APIs that are above our targeted browser versions, and for these, we have polyfills for all but very old browsers).

I've got other ideas on the README of things I'd like to see done at some point, especially a privilege manager, and of course passport-next integration.

If you were open to considering it for the project umbrella here, it might be a good time to take a look.

Hope all is well by you and everyone here... Take care!

@rwky
Copy link

rwky commented Apr 2, 2020

Sounds good. I'll review at some point can't say when though the pandemic is causing havoc at work right now.

brettz9 added 8 commits July 6, 2020 14:55
* commit 'ad4f18ab7d55952612f081caa054a768f288971e':
  Updated npm deps
  Updated skel
  Updated funding.yml
  Added FUNDING.yml any other devs feel free to add yourself
  v3.1.0
  Updated npm deps
  Feature: Pass instantiated strategy to authenticate.

# Conflicts:
#	lib/middleware/authenticate.js
#	package-lock.json
#	package.json
#	templates/package.json.j2
* master:
  Updated travis node versions

# Conflicts:
#	.travis.yml
- npm: Update devDeps
- npm: Update devDeps (including swapping eslint-config-ash-nazg peerDep. and drop need for typescript)
…plugin-markdown` and `eslint-plugin-jsdoc`

- npm: Update devDeps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promises
3 participants