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

[v12.x backport] util: use a global symbol for util.promisify.custom #32349

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Mar 18, 2020

This backports #31672 to Node v12.

It has already been backported to Node v13 as 975d6b0.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added util Issues and PRs related to the built-in util module. v12.x labels Mar 18, 2020
@codebytere codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57
@MylesBorins
Copy link
Contributor

I got a rebase of this working but was unable to push to the branch. You can find it at https://github.com/MylesBorins/node/tree/backport/v12.x/lib/util/use-global-util-promisify-custom-symbol

Define `util.promisify.custom`
as `Symbol.for("nodejs.util.inspect.custom")`, rather than
as `Symbol("util.inspect.custom")`.

This allows custom `promisify` wrappers to easily/safely be defined
in non‑Node.js environments.

Fixes: nodejs#31647

PR-URL: nodejs#31672
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@ExE-Boss ExE-Boss force-pushed the backport/v12.x/lib/util/use-global-util-promisify-custom-symbol branch from e874d47 to 0a8c5c1 Compare April 1, 2020 07:11
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

MylesBorins pushed a commit that referenced this pull request Apr 2, 2020
Define `util.promisify.custom`
as `Symbol.for("nodejs.util.inspect.custom")`, rather than
as `Symbol("util.inspect.custom")`.

This allows custom `promisify` wrappers to easily/safely be defined
in non‑Node.js environments.

Fixes: #31647

Backport-PR-URL: #32349
PR-URL: #31672
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

landed in 32c0449

@MylesBorins MylesBorins closed this Apr 2, 2020
@targos
Copy link
Member

targos commented Apr 8, 2020

@MylesBorins it's a very small addition but the original PR was semver-minor. This just landed on the v12.16.2 patch release though.

@ExE-Boss ExE-Boss deleted the backport/v12.x/lib/util/use-global-util-promisify-custom-symbol branch April 9, 2020 11:53
@MylesBorins
Copy link
Contributor

@targos oof

Do you think we should revert?

@targos
Copy link
Member

targos commented Apr 9, 2020

We can talk about it in 5 minutes, but no I don't think we should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants