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

[New] use a global symbol for util.promisify.custom #19

Merged

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Feb 8, 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 and for non‑Node promisify implementations to be interoperable.

See also:

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
Copy link
Owner

ljharb commented Feb 8, 2020

I'm not particularly enthusiastic about this change; the custom symbols should not work in a non-node environment.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 8, 2020

Except that the  custom symbols will  work in a non‑Node environment that has native symbol support.

This change just ensures that in a setup which has different implementations of util.promisify, they all end up being interoperable.

@ExE-Boss ExE-Boss force-pushed the feat/use-global-util-promisify-custom-symbol branch from 45fac72 to e4a1d5b Compare February 8, 2020 14:09
@ljharb
Copy link
Owner

ljharb commented Feb 8, 2020

There shouldn’t be different implementations of it - in new node, it should be node’s, only; in old node, this one only; and in browsers, there should only be one shim/polyfill on the page.

@ljharb
Copy link
Owner

ljharb commented Jan 6, 2021

@ExE-Boss i think i need to be added to this fork as well?

@ExE-Boss ExE-Boss force-pushed the feat/use-global-util-promisify-custom-symbol branch from e4a1d5b to 77319f0 Compare January 6, 2021 08:30
@ExE-Boss ExE-Boss closed this Jan 6, 2021
@ExE-Boss ExE-Boss reopened this Jan 6, 2021
@ExE-Boss ExE-Boss closed this Jan 6, 2021
@ExE-Boss ExE-Boss reopened this Jan 6, 2021
@ExE-Boss ExE-Boss force-pushed the feat/use-global-util-promisify-custom-symbol branch from 77319f0 to 2b9f90e Compare January 6, 2021 12:21
@ljharb ljharb force-pushed the feat/use-global-util-promisify-custom-symbol branch from 2b9f90e to 8f8631b Compare January 6, 2021 21:44
@ljharb ljharb changed the title feat: use a global symbol for util.promisify.custom [New] use a global symbol for util.promisify.custom Jan 6, 2021
@ljharb ljharb merged commit 8f8631b into ljharb:master Jan 6, 2021
@ExE-Boss ExE-Boss deleted the feat/use-global-util-promisify-custom-symbol branch January 10, 2021 14:52
@@ -16,6 +15,7 @@
"aud": "^1.1.3",
"auto-changelog": "^2.2.1",
"eslint": "^7.17.0",
"has-symbols": "^1.0.1",
Copy link
Contributor Author

@ExE-Boss ExE-Boss Jan 10, 2021

Choose a reason for hiding this comment

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

@ljharb It just occurred to me that this change is wrong, because has‑symbols is used in implementation.js:

var hasSymbols = require('has-symbols')();

This was fixed in #25

Copy link
Owner

Choose a reason for hiding this comment

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

Indeed; it’s been fixed in #25.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants