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

Remove support for callbacks in models #48

Closed
jwerre opened this issue Oct 16, 2021 · 7 comments · Fixed by #190
Closed

Remove support for callbacks in models #48

jwerre opened this issue Oct 16, 2021 · 7 comments · Fixed by #190
Labels
backwards breaking ✂️ This change will not work with the current version of the module. enhancement ✨ New feature or request

Comments

@jwerre
Copy link
Contributor

jwerre commented Oct 16, 2021

Callback support adds unneeded complexity. Models should be exclusively promise based version ^4.1.0. #20 (comment)

@jankapunkt
Copy link
Member

I agree, however

  • this will be breaking, thus a new major release imo
  • all tests will have to be manually checked against false-positives and false-negatives
  • some tests may require to be updated
  • if we use promises we can also safely write async functions as they should be natively supported with node >= 12
  • then we can also use async functions for async mocha units
  • I still would like to get the release management issue resolved first

@jwerre
Copy link
Contributor Author

jwerre commented Oct 18, 2021

@jankapunkt By next major version do you mean v5? As far as I understand that's the migration to TypeScript correct? If thats the case it would be nice to do this before all that happens. I think removing support for callbacks can happen now, as we move from v3 to v4.

We don't really have to do much, just update the documentation and perhaps put a warning message if people try to use callbacks: "support for callbacks has been deprecated—we recommend using Promises in your models since this may have adverse effects in future minor releases". Since we've officially removed callback support at v4 and have given people fair warning we should be ok to remove callbacks at any time during the v4 lifecycle.

@HappyZombies HappyZombies added enhancement ✨ New feature or request backwards breaking ✂️ This change will not work with the current version of the module. labels Oct 18, 2021
@jankapunkt
Copy link
Member

If we remove callbacks completely and it would be breaking changed then it would have to be a major version. Usually versioning follows implementation so if we imement something breaking before TS it would become 5.0.0 and Typescript may be a later version like 6.0.0

@jankapunkt
Copy link
Member

Vice verca this means if we don't break big changes then we can easily do this as 4.1.0

This was referenced Nov 5, 2021
@jwerre
Copy link
Contributor Author

jwerre commented Nov 15, 2021

@jankapunkt, @HappyZombies I found some time to work on this a bit today. I removed bluebird and promisify-any on the abstract-grant-type.js and restricted the tests.

If you get some time take a look and let me know if you have any questions/concerns: 4a360ee. If all looks good I'll continue to chip away at this.

@jankapunkt
Copy link
Member

@jwerre do you mind open a draft PR so we don't forget you working on this? I will check it out later but I want to focus on some of the security-related issues for the next days.

@jorenvandeweyer
Copy link
Member

Callback support dropped in 5.0.0 see #190

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. enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants