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 #7

Open
ZachHaber opened this issue Sep 12, 2018 · 8 comments · May be fixed by #21
Open

Promises #7

ZachHaber opened this issue Sep 12, 2018 · 8 comments · May be fixed by #21
Labels
enhancement New feature or request

Comments

@ZachHaber
Copy link

I saw this fork from the network graph on github's insights tab, and from the name and the current frequency of commits, it seems that this planning on continuing support/development of passport.
I would like to throw in my vote (whatever that counts for) to support promises.
Looking forward to seeing this develop further! 😃

@guyellis
Copy link

@rwky I haven't looked at the fork that @ZachHaber is referencing. A popular way of doing a non-breaking transition (or dual implementation) of callbacks and promises is to return a promise if a callback is missing. I know that's an oversimplification... might be viable.

@ZachHaber - do you have a link to the fork that has promises - how complete is it?

@ZachHaber
Copy link
Author

Sorry if I got the terminology wrong, I was referring to this AKA passport-next/passport (which is a fork of jaredhanson/passport). I suppose it is a proper Javascript issue to fail to bind "this" correctly 😛. Let's just hope this gets the right closure 😉 .
I was thinking that the dual implementation makes sense that way this is still capable of being easily substituted instead of jaredhanson.
I did have one other question: What are you planning to solve/improve/change from the original passport.js that is the reason this fork exists?

@guyellis guyellis added enhancement New feature or request question Further information is requested labels Sep 13, 2018
@guyellis
Copy link

@ZachHaber the existing jaredhanson repos aren't accepting PRs anymore. This usually happens when a project has been abandoned and the original owners no longer have time to continue work on it.

@rwky
Copy link

rwky commented Sep 13, 2018

I love promises and I think integrating them in to passport would be great. It won't be a small job I think we should complete linting #3 first so then the code is at least modern, then we can look at promises.

@rwky rwky added help wanted Extra attention is needed and removed question Further information is requested labels Sep 13, 2018
@guyellis
Copy link

@ZachHaber - I agree with @rwky - we should get linting in place before we tackle promises. If you feel like tackling that task and create a PR that adds linting then that will get us one step closer to promises. I've created #8 to add linting to this project. Feel free to tackle that if you want.

@guyellis
Copy link

@rwky another approach we could take with promises is to make it a breaking change to 2.x and drop the callbacks from that major. That would simplify the approach because we wouldn't be supporting 2 styles.

Advantages:

  • Simpler codebase which means fewer bugs and less work when making changes which in turn will lead to a higher quality codebase.

Disadvantages:

  • No easy/direct migration for existing users to version 2.x.
  • Might need to back port fixes to the 1.x series for users still using callbacks and users wanting to migrate to this codebase.

@rwky
Copy link

rwky commented Sep 13, 2018

I'd rather not switch to full blow promises yet. I'd probably do something like this:

  • Add in promises, deprecate callbacks, update to 2.x
  • Give it a while then release 3.x with callbacks removed

That way we only have one version at a time to maintain and people have a while to upgrade (disclaimer: I've some code that's running this module I wouldn't want to upgrade to promises right now).

Once node 6 is EOL (April 2019) I imagine promise support might boost a bit across modules since everyone will then have access to async/await.

@rwky
Copy link

rwky commented Sep 18, 2018

Now linting has been done we can look at this 👍

@rwky rwky mentioned this issue May 9, 2019
5 tasks
@brettz9 brettz9 linked a pull request Jun 4, 2019 that will close this issue
5 tasks
@brettz9 brettz9 removed the help wanted Extra attention is needed label Jul 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants