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

Use native promises and async/await, drop bluebird and promisify-any #190

Merged
merged 7 commits into from
Jun 22, 2023

Conversation

jankapunkt
Copy link
Member

@jankapunkt jankapunkt commented Jun 9, 2023

Summary

This is a breaking change!

  • Removes all bluebird and promisify-any dependencies.
  • Uses async/await in favour of Promise where applicable
  • Removes callback support
  • updated tests
  • improved readability and structure
  • replace Promise. calls in async functions with native behaviour
    • use await chain instead of Promise.all
    • throw error instead of Promise.reject
    • return values instead of Promise.resolve
    • some tests still use Promise.resolve in combination with sinon stubs but that's because in this case the pattern is more concise than an async function counterpart

Relation to the prior PR

There was a prior PR that did basically the same as this one: #107

The development branch became too different from #107 and the merge diff was unbearable to read. I closed #107 and used it as blueprint to implement it from the latest development state.

Release proposal

If this gets accepted and merged we should create a 5.0.0-rc.0 release to allow early adopters to integrate and report any issues beyond the expected ones. Please comment.

Linked issue(s)

#19
#20
#15
#48

Involved parts of the project

  • grant types
  • handlers
  • utils
  • OAuth2Server class
  • tests

Added tests?

  • tests updated

OAuth2 standard

  • no changes regarding standard

Reproduction

  • clone, run tests
  • create local package and integrate with own project

@jankapunkt jankapunkt added code quality 🧽 Relating to code quality backwards breaking ✂️ This change will not work with the current version of the module. labels Jun 9, 2023
@jankapunkt
Copy link
Member Author

@Uzlopak @HappyZombies @jorenvandeweyer friendly ping - would one of you be willing to take a look at this one, maybe discuss details?

I am still unsure if dropping callback support entirely is a good thing, we have already some dependants that might want a slower transition.

We basically have two options:

A - remove callback support (as already done in this PR) and immediately bump a major step to 5.0.0 (but provide an rc release first to allow early adopters to test)

B - An alternative would be to keep callback support for now and raise a warning that it will be removed in the next major version?

Copy link
Member

@jorenvandeweyer jorenvandeweyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favour of removing callback support and releasing a 5.0.0-rc.0.

This makes the transition to typescript a lot easier!

I do see something inconsistent in the PR.

Sometimes the Promise.all is replaced by a chain of awaits and sometimes it's not. Maybe we can remove all Promise.all functions?

@jankapunkt
Copy link
Member Author

@jorenvandeweyer thanks a lot for your opinion on this and the hint to the Promise.all.

Regarding the callbacks - I tend towards this approach, too!

Regarding Promise.all - I will remove them entirely. I also think there were issues with Promise race conditions by using Promise.all (mentioned deep in the conversations of one of the issues....). Replacing this with chains of awaits woulf automatically prevent unintended execution order.

@jankapunkt
Copy link
Member Author

@HappyZombies @Uzlopak do you generally agree with the aforementioned proceeding?

remove callback support (as already done in this PR) and immediately bump a major step to 5.0.0 (but provide an rc release first to allow early adopters to test)

If this is merged I would publish 5.0.0-rc.0 and raise awareness for migration. wdyt?

@jankapunkt
Copy link
Member Author

@HappyZombies @Uzlopak @jorenvandeweyer some update on this. I manually wrote a few emails to repo owners that I found in our dependants list. I asked them about their perspective on this and the vast majority is fine with going full async and dropping callbacks in 5.x. I will therefore continue the proposed strategy and release the 5.0.0-rc.0 this week

@HappyZombies
Copy link
Member

@jankapunkt thanks for reaching out to devs about this. Although I think it might be possible to support both promise and a callback (I have seen this before in other modules I've used), it's obviously just more effort. If we're good with removing callback support entirely, then let's do it 👍

@jankapunkt jankapunkt merged commit 3d766a7 into development Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards breaking ✂️ This change will not work with the current version of the module. code quality 🧽 Relating to code quality
Projects
None yet
3 participants