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.

Sign up for GitHub

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

Jump to bottom

util: use a global symbol for util.promisify.custom #31672

Closed
wants to merge 3 commits into from
Closed

util: use a global symbol for util.promisify.custom #31672

wants to merge 3 commits into from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Feb 7, 2020 •
edited
Loading

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 and for non‑Node promisify implementations to be interoperable.

Fixes: #31647

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

@ExE-Boss
util: Use a global symbol for util.promisify.custom
5a96702 
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
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 7, 2020
@addaleax
Copy link
Member

addaleax commented Feb 7, 2020

@ExE-Boss Do you think you could add documentation/tests here? You could take a look at dadd6e1 for inspiration. Basically, there should be one example of using this way of accessing the symbol in the docs, and one in the tests.

@addaleax addaleax added lts-watch-v12.x semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 7, 2020
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 7, 2020

I’m waiting for CI to finish, so that I know which tests to update.

@ExE-Boss ExE-Boss changed the title util: Use a global symbol for util.promisify.custom util: use a global symbol for util.promisify.custom Feb 7, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28980/

jasnell
jasnell reviewed Feb 7, 2020
View reviewed changes
doc/api/util.md
-->

* {symbol} that can be used to declare custom promisified variants of functions,
see [Custom promisified functions][].

In addition to being accessible through `util.promisify.custom`, this
symbol is [registered globally][global symbol registry] and can be
accessed in any environment as `Symbol.for('nodejs.util.promisify.custom')`.
Copy link
Member

Choose a reason for hiding this comment

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

It should be noted that any code that uses Symbol.for('nodejs.util.promisify.custom') syntax rather than the util.promisify.custom accessor will not work properly on older versions of Node.js so if compatibility with older versions is important, use of the accessor is preferred.

Copy link
Contributor Author

@ExE-Boss ExE-Boss Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That will be visible in the History drop‑down, same as with util.inspect.custom, which doesn’t mention this either.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer a more explicit comment on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recommend doing that in its own PR, as it will affect the documentation of util.inspect.custom (and possibly other symbols) as well.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 8, 2020 •
edited
Loading

We should probably ensure that all methods which use the internal customPromisifyArgs symbol get passed to util.promisify before any user code runs, to ensure that any shims that use Symbol.for("nodejs.util.promisify.custom") do not break built‑in functions, ex.: @browserify’s util shim, which doesn’t even implement the customPromisifyArgs codepath, or @ljharb’s util.promisify, which exposes its own version of the customPromisifyArgs symbol and associated codepath, which is incompatible with Node’s.

ExE-Boss added a commit to ExE-Boss/util.promisify that referenced this pull request Feb 8, 2020
@ExE-Boss
feat: use a global symbol for util.promisify.custom
45fac72 
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.

Refs: nodejs/node#31647
Refs: nodejs/node#31672
@ExE-Boss ExE-Boss mentioned this pull request Feb 8, 2020
Merged
@ExE-Boss ExE-Boss requested a review from ljharb February 8, 2020 00:22
@ExE-Boss ExE-Boss mentioned this pull request Feb 8, 2020
Open
@ljharb
Copy link
Member

ljharb commented Feb 8, 2020

My util.promisify package only exposes a new symbol if util.promisify.custom doesn't already exist; there's no issue here.

ExE-Boss added a commit to EB-Forks/node-util that referenced this pull request Feb 8, 2020
@ExE-Boss
feat: use a global symbol for util.promisify.custom
ec9c70f 
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.

Refs: nodejs/node#31647
Refs: nodejs/node#31672
@ExE-Boss ExE-Boss mentioned this pull request Feb 8, 2020
Open
ExE-Boss added a commit to ExE-Boss/util.promisify that referenced this pull request Feb 8, 2020
@ExE-Boss
feat: use a global symbol for util.promisify.custom
e4a1d5b 
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.

Refs: nodejs/node#31647
Refs: nodejs/node#31672
ExE-Boss added a commit to EB-Forks/node-util that referenced this pull request Feb 8, 2020
@ExE-Boss
feat: use a global symbol for util.promisify.custom
f89af7d 
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.

Refs: nodejs/node#31647
Refs: nodejs/node#31672
@addaleax addaleax closed this Mar 12, 2020
@ExE-Boss ExE-Boss deleted the lib/util/use-global-util-promisify-custom-symbol branch March 13, 2020 02:32
@ExE-Boss
Copy link
Contributor Author

Should this be backported to v12.x and v13.x just like how #20857 (comment) was?

BridgeAR pushed a commit that referenced this pull request Mar 17, 2020
@ExE-Boss @BridgeAR
util: use a global symbol for util.promisify.custom
975d6b0 
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

PR-URL: #31672
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@ExE-Boss ExE-Boss mentioned this pull request Mar 18, 2020
Closed
4 tasks
MylesBorins added a commit that referenced this pull request Mar 19, 2020
@MylesBorins
2020-03-24 Version 13.12.0 (Current)
8fc5a91 
Notable changes:

* [a44da56] - (SEMVER-MINOR) doc: update stability of report features (Colin Ihrig) #32242
* [306ed96] - (SEMVER-MINOR) doc,lib,src,test: make --experimental-report a nop (Colin Ihrig) #32242
* [ea7f89d] - (SEMVER-MINOR) test: remove common.skipIfReportDisabled() (Colin Ihrig) #32242
* [3f1f518] - (SEMVER-MINOR) build: make --without-report a no-op (Colin Ihrig) #32242
* [36ab39f] - (SEMVER-MINOR) build: remove node_report option in node.gyp (Colin Ihrig) #32242
* [514b7c2] - (SEMVER-MINOR) src: unconditionally include report feature (Colin Ihrig) #32242
* [435fbbc] - (SEMVER-MINOR) worker: allow URL in Worker constructor (Antoine du HAMEL) #31664
* [975d6b0] - (SEMVER-MINOR) util: use a global symbol for `util.promisify.custom` (ExE Boss) #31672
@MylesBorins MylesBorins mentioned this pull request Mar 19, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
@ExE-Boss @MylesBorins
util: use a global symbol for util.promisify.custom
53fd0d8 
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

PR-URL: #31672
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 24, 2020
@MylesBorins
2020-03-25 Version 13.12.0 (Current)
968751d 
Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig) [#32309](#32309)
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* node\_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 24, 2020
@MylesBorins
2020-03-25 Version 13.12.0 (Current)
27dbe1f 
Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig) [#32309](#32309)
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* node\_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 25, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
a87ac59 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
65782ff 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
f6a339f 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
113c04c 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 1, 2020
@ExE-Boss @MylesBorins
util: use a global symbol for util.promisify.custom
673cd60 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Apr 1, 2020
@ExE-Boss
util: use a global symbol for util.promisify.custom
0a8c5c1 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 2, 2020
@ExE-Boss @MylesBorins
util: use a global symbol for util.promisify.custom
32c0449 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Apr 3, 2020
Merged
ExE-Boss added a commit to ExE-Boss/util.promisify that referenced this pull request Jan 6, 2021
@ExE-Boss
feat: use a global symbol for util.promisify.custom
77319f0 
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.

Refs: nodejs/node#31647
Refs: nodejs/node#31672
ExE-Boss added a commit to ExE-Boss/util.promisify that referenced this pull request Jan 6, 2021
@ExE-Boss
feat: use a global symbol for util.promisify.custom
2b9f90e 
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.

Refs: nodejs/node#31647
Refs: nodejs/node#31672
ljharb pushed a commit to ExE-Boss/util.promisify that referenced this pull request Jan 6, 2021
@ExE-Boss @ljharb
[New] use a global symbol for util.promisify.custom
8f8631b 
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.

Refs: nodejs/node#31647
Refs: nodejs/node#31672
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@devsnek devsnek devsnek approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@targos targos Awaiting requested review from targos

@TimothyGu TimothyGu Awaiting requested review from TimothyGu

@ljharb ljharb Awaiting requested review from ljharb

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support util.promisify.custom as a global symbol
9 participants
@ExE-Boss @addaleax @nodejs-github-bot @ljharb @jasnell @cjihrig @devsnek @BridgeAR @MylesBorins