-
-
Notifications
You must be signed in to change notification settings - Fork 7
[New] use a global symbol for util.promisify.custom
#19
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
Conversation
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
|
I'm not particularly enthusiastic about this change; the custom symbols should not work in a non-node environment. |
|
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 |
45fac72 to
e4a1d5b
Compare
|
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. |
|
@ExE-Boss i think i need to be added to this fork as well? |
e4a1d5b to
77319f0
Compare
77319f0 to
2b9f90e
Compare
2b9f90e to
8f8631b
Compare
util.promisify.customutil.promisify.custom
| "aud": "^1.1.3", | ||
| "auto-changelog": "^2.2.1", | ||
| "eslint": "^7.17.0", | ||
| "has-symbols": "^1.0.1", |
There was a problem hiding this comment.
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:
util.promisify/implementation.js
Line 25 in 8980c55
| var hasSymbols = require('has-symbols')(); |
This was fixed in #25
There was a problem hiding this comment.
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.
Define
util.promisify.customas:rather than as:
This allows custom
promisifywrappers to easily/safely be defined in non‑Node.js environments and for non‑Nodepromisifyimplementations to be interoperable.See also:
util.promisify.customas a global symbol nodejs/node#31647util.promisify.customnodejs/node#31672