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

Dropping callbacks support in v7 #11431

Closed
AbdelrahmanHafez opened this issue Feb 19, 2022 · 10 comments
Closed

Dropping callbacks support in v7 #11431

AbdelrahmanHafez opened this issue Feb 19, 2022 · 10 comments
Labels
discussion If you have any thoughts or comments on this issue, please share them!
Milestone

Comments

@AbdelrahmanHafez
Copy link
Collaborator

Mongoose's codebase can become much simpler if we're able to use async/await, one reason that can make it a bit tricky is that we still need to support callbacks.

That made sense until v6, but the maintenance cost now significantly outweighs the value returned by it, there should be no one using callbacks out there.

I think we should introduce a major breaking change in v7 to remove support for callbacks.
If we agree on going in that direction, I think we can start working on v7 right away, the codebase simplification should make it much more accessible for newer contributors.
What do you think?
@vkarpov15 @Uzlopak @IslandRhythms @ahmedelshenawy25

@AbdelrahmanHafez AbdelrahmanHafez added the discussion If you have any thoughts or comments on this issue, please share them! label Feb 19, 2022
@Uzlopak
Copy link
Collaborator

Uzlopak commented Feb 19, 2022

It is also a small performance bottleneck as we always have to artificially call nextTick.
And we could create Errors when we need them and not before hand to keep the stack trace. Reminds me of #9127 Would definetily reduce overhead.

@vkarpov15
Copy link
Collaborator

In general, I think this is a good idea. I'd like to revisit this later before we commit to doing it for v7, but I think we likely will end up dropping callback support. Especially if async stack traces become more standardized. If anyone has strong feelings about this, please post them 👍

@AbdelrahmanHafez
Copy link
Collaborator Author

Sounds good.

How do you feel about shipping major versions with breaking changes more frequently instead of large major versions with many breaking changes?

For example, when we decide it's a good time to get rid of callbacks, there is no reason for us not to keep the codebase as is, get rid of callbacks, and ship v7.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Feb 26, 2022

People need continuity. Too many major version jumps are disencouraging to use such a supposedly "unstable" product. Look at versioning of chrome and Firefox. They bump the major very fast but actually nobody cares because 99.9% of the users are end-users, who don't care. But if the consumers would be more devs, than versioning would be under more review...

Well I actually recommend to make a concept, regarding deprecating. First step should be some common rule. Like for deprecation we will have 6, 12 or 18 months notice period. This means we potentially mark specific API as deprecated. And in the Changelog we notice that the API got deprecated. Maybe some other place we also notice what code we will deprecate.
This avoids, that somebody complains that he was not informed earlier. You know the drill.

Then if we want to deprecate code we look what code is ripe to be thrown out and then create the PR and bump the major. If multiple things are ripe, we can do them together in a major. E.g.we see in 2 weeks another thing can be kicked out we just wait 2 weeks and voila, nice and shiny new major.

@AbdelrahmanHafez
Copy link
Collaborator Author

Well said.

We can start logging a deprecation warning for using callbacks in 6.3, and offer an option to suppress those warnings.
That should give users a fair amount of time to switch to async/await.

@vkarpov15
Copy link
Collaborator

I like the idea of a deprecation warning for this particular change. Too many deprecation warnings cause friction, but in this case I'd rather give people ample time to either migrate their code or loudly complain about this change being a terrible idea. In the latter case, we can consider keeping callbacks - they don't add too much friction for maintenance currently. They just make the API a little harder to understand, and may cause problems for async stack traces if that becomes more standardized.

@AbdelrahmanHafez
Copy link
Collaborator Author

@vkarpov15
Do you have an idea if there is a planned timeline on when async stack traces may be more standardized?

@vkarpov15
Copy link
Collaborator

I don't. As far as I know, async stack traces are a V8 feature (https://v8.dev/docs/stack-trace-api) and not necessarily stable. I'd love to make it possible to write a plugin to use async stack traces, in conjunction with clean-stack, I think that would be the neatest way of adding support for async stack traces without risking us getting into a situation where Mongoose guarantees behavior that changes in a new Node version.

@hasezoey
Copy link
Collaborator

hasezoey commented Feb 23, 2023

with #12955 callbacks have been removed for mongoose 7.0, the only thing left to-do is to add a deprecation notice in 6.x (because from what i can tell, there are none yet)

@hasezoey hasezoey added this to the 7.0 milestone Feb 23, 2023
@vkarpov15
Copy link
Collaborator

I think we'll pass on the deprecation warning given that we've already implemented dropping callbacks, and 7.0 has error messages in place if you try to use callbacks. We'll close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion If you have any thoughts or comments on this issue, please share them!
Projects
None yet
Development

No branches or pull requests

4 participants