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

Deprecate multipleResolves? #41554

Closed
benjamingr opened this issue Jan 16, 2022 · 14 comments · Fixed by #41872
Closed

Deprecate multipleResolves? #41554

benjamingr opened this issue Jan 16, 2022 · 14 comments · Fixed by #41872
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises.

Comments

@benjamingr
Copy link
Member

benjamingr commented Jan 16, 2022

I'm thinking, should we deprecate the multipleResolves event? Is it used?

We've known it had problems shortly after it was added and it did not end up being useful for the use cases it was initially designed for.

Also cc @nodejs/tsc since deprecating APIs presumably needs TSC approval/discussion?

@benjamingr benjamingr added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises. labels Jan 16, 2022
@benjamingr
Copy link
Member Author

@nodejs/diagnostics @nodejs/promises-debugging (if anyone on that team is still active :D)

@mcollina
Copy link
Member

I'm ok for removing it.

@jasnell
Copy link
Member

jasnell commented Jan 16, 2022

+1 for deprecating

@mmarchini
Copy link
Contributor

I'll poke around next week to see if anyone at Netflix or elsewhere is using it (and if so, will try to understand the use case). Personally I haven't used it at all.

It would be nice if we could get data from npm to see if anyone is using it and how. Also, maybe a Twitter poll (not a very scientific method but we have used in the past).

I don't remember the issues regarding multipleResolves, do we have it listed anywhere?

@ronag
Copy link
Member

ronag commented Jan 16, 2022

+1

@Mesteery
Copy link
Contributor

Mesteery commented Jan 16, 2022

@mmarchini
Copy link
Contributor

@Mesteery GitHub Code Search has a waiting list, so folks can't access the URL unless they already joined :/

Either way, logging is a valid use case IMO. Although 45 files are an indication that it's an underused feature.

@benjamingr I don't remember if this was something that could be implemented in userland or not. Is it?

@benjamingr
Copy link
Member Author

@benjamingr I don't remember if this was something that could be implemented in userland or not. Is it?

It's not, it's something V8 did just for Node.js and I regret wasting their good-will and time on the feature only to find out at the end there are a ton of cases they resolve promises multiple times in their implementations of stuff like Promise.all / Promise.race which means the hook's value is significantly diminished in practice.

If the hook worked it would have been pretty useful.

Either way, logging is a valid use case IMO. Although 45 files are an indication that it's an underused feature.

I went through all of them and I think the vast majority are just examples of the hook rather than genuine "everyday" usage of the hook. I am happy to also send out an unofficial survey of "Are you familiar with multipleResolves" and "Are you using multipleResolves" if that'd help.

@mmarchini
Copy link
Contributor

It's not, it's something V8 did just for Node.js and I regret wasting their good-will and time on the feature only to find out at the end there are a ton of cases they resolve promises multiple times in their implementations of stuff like Promise.all / Promise.race which means the hook's value is significantly diminished in practice.

If the hook worked it would have been pretty useful.

Got it. Functionality-wise this seems like something that can be deprecated and removed. I think it's worth adding this context to the OP so others can also make an informed decision.

I went through all of them and I think the vast majority are just examples of the hook rather than genuine "everyday" usage of the hook

That's a relief since reduces the chances of open source packages propagating usage of it. There still might be some relevant usage in private repos that could surface from asking the community.

I am happy to also send out an unofficial survey of "Are you familiar with multipleResolves" and "Are you using multipleResolves" if that'd help

It doesn't have to be unofficial, we can share one through the Node.js account. It can be a twitter poll ("yes, i'm familiar and use", "yes, I'm familiar but don't use", "I'm not familiar but would use", "I'm not familiar and wouldn't use", or just "would you be opposed to deprecating multipleResolves (yes/no)? If no please reach out at #41554 with your use case"), not a sophisticated survey (yes, I'm volunteering to run this poll).

@benjamingr
Copy link
Member Author

benjamingr commented Jan 17, 2022

It doesn't have to be unofficial, we can share one through the Node.js account. It can be a twitter poll ("yes, i'm familiar and use", "yes, I'm familiar but don't use", "I'm not familiar but would use", "I'm not familiar and wouldn't use", or just "would you be opposed to deprecating multipleResolves (yes/no)? If no please reach out at #41554 with your use case"), not a sophisticated survey (yes, I'm volunteering to run this poll).

Go for it, I would be interested in what the result would be.

Edit: I'll also run one on the Node.js user group in Facebook I moderate to get some non-twitter data

@RaisinTen
Copy link
Contributor

+1 with the deprecation. FWIW, we don't use this in Postman.

@benjamingr
Copy link
Member Author

benjamingr commented Jan 18, 2022

So some data from the unofficial poll (asked in the Israeli Node.js group on Facebook):

  • 90 people have answered
  • 95.5% answered they are not familiar with multipleResolves
  • 4.5% answered they know of multipleResolves but have not used it.
  • 0% said they know of it and have used it.

image


Edit: gave it more time, results are similar. Were you able to run a twitter survey by any chance @mmarchini ?

image

@mmarchini
Copy link
Contributor

@benjamingr was trying to figure out how to share a poll with the Node.js account. Just opened an issue in the appropriate repo for folks managing that account to tweet it.

benjamingr added a commit to benjamingr/io.js that referenced this issue Feb 6, 2022
Deprecate the process multipleResolves event to detect when a promise is
resolved more than once because it never really worked.

Fixes: nodejs#41554
benjamingr added a commit to benjamingr/io.js that referenced this issue Feb 6, 2022
Deprecate the process multipleResolves event to detect when a promise is
resolved more than once because it never really worked.

Fixes: nodejs#41554
@benjamingr
Copy link
Member Author

image

Looks like the twitter results are similar (there is a minority of "currently using" but 0 comments of how and a big majority for "unfamiliar")

nodejs-github-bot pushed a commit that referenced this issue Feb 8, 2022
Deprecate the process multipleResolves event to detect when a promise is
resolved more than once because it never really worked.

Fixes: #41554

PR-URL: #41872
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Deprecate the process multipleResolves event to detect when a promise is
resolved more than once because it never really worked.

Fixes: nodejs#41554

PR-URL: nodejs#41872
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Deprecate the process multipleResolves event to detect when a promise is
resolved more than once because it never really worked.

Fixes: nodejs#41554

PR-URL: nodejs#41872
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Deprecate the process multipleResolves event to detect when a promise is
resolved more than once because it never really worked.

Fixes: nodejs#41554

PR-URL: nodejs#41872
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants