-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Improve async dispose fallback #16409
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56800 |
"x asyncDispose body", | ||
"body", | ||
"y dispose promise body", | ||
]); |
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.
In current main, the y dispose promise body
is printed before x asyncDipose body
(REPL)
} | ||
if (typeof dispose !== "function") { | ||
throw new TypeError(`Property [Symbol.dispose] is not a function.`); | ||
throw new TypeError( |
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.
Maybe just throw Invalid dispose function
.
Because "Property [Symbol.dispose] is not a function."
it is also possible for the user to add a [Symbol.asyncDispose]
.
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.
Okay, how about "Object is not disposable.", since "dispose" is a verb.
@@ -30,18 +45,24 @@ export default function _usingCtx() { | |||
// core-js-pure uses Symbol.for for polyfilling well-known symbols | |||
if (isAwait) { | |||
var dispose = | |||
value[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")]; | |||
value[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")], |
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.
If Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")
is undefined
, which implies that the asyncDispose
symbol is not properly polyfilled, we can provide a better error such as "Symbol.asyncDispose
is not defined" rather than throwing "object is not disposable" as we are reading the "undefined"
property of the value
.
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.
I prefer to unify them into something like disposal function is invalid
. :)
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.
The disposal function may be okay. The proposed error here is for the case when Symbol.asyncDispose
is not defined, rather than resource[Symbol.asyncDispose]
is not defined.
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.
You are right! I changed it to invalid
.
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.
I just realized that Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")
will always return a symbol. If Symbol.dispose
is not polyfilled, user will end up defining a disposal function under the name undefined
without any runtime error. So if we can't find the disposal function and value.undefined
is a function, this suggests that the user may not polyfill Symbol.dispose
and in this case we can provide a better error message. Anyway it will be addressed in another PR.
I think we can add a section to the plugin docs about how to use this plugin with core-js 3 or other polyfill providers. Integration tests with polyfill providers should be added as well.
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.
Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")
always returns a symbol, so shouldn't we always look for value[symbol]
?
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.
In chrome 122 or any browsers without @@dispose
support,
const disposable = { [Symbol.dispose]() {} }
will be evaluated as if it were defined as
const disposable = { undefined() {} }
If users have properly polyfilled Symbol.dispose
, the transformed result will be evaluated as if
const disposable = { [Symbol.for("Symbol.dispose")]() {} }
The usingCtx
helper work when 1) the browser has native support of @@dispose
or 2) the @@dispose
has been polyfilled correctly. However, in the case when a polyfill is missing, the helper will throw object is not disposable
because the desired disposal function is defined at the undefined
property, which can be very confusing for end users because they thought they have defined it, without realizing that the symbol is not polyfilled. In this case we should suggest users that @@dispose
may not be properly polyfilled.
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.
Makes sense! But given that this exists in all symbol-related helpers, I'm a little hesitant that we should include it by default.
Not related to this PR: return (async function () {
const log = [];
async function f() {
using y = {
[Symbol.dispose || Symbol.for("Symbol.dispose")]() {
log.push("y");
},
};
// Commenting me will make the test fail
await using x = {
[Symbol.dispose || Symbol.for("Symbol.dispose")]() {},
};
log.push("f body");
}
const promise = f();
log.push("body");
await promise;
expect(log).toEqual(["f body", "body", "y"]);
})(); |
Yes and we can definitely add more behaviour tests. As for your example, any sync disposable declared before an async disposable will be disposed after the async disposable is disposed, which happens no earlier than the next microtick, so |
AWAIT_ASYNC = 3, | ||
AWAIT_USING_DISPOSE = 2, | ||
// Flag.AWAIT_USING | Flag.ASYNC_DISPOSE | ||
AWAIT_USING_ASYNC_DISPOSE = 3, |
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.
I renamed the enums so that hopefully two-year-later me will not get lost when reviewing the helper.
Previously we throw "[Symbol.dispose] is not a function", which can be very confusing.
Co-authored-by: Nicolò Ribaudo <hello@nicr.dev>
0f4e40a
to
c3a7acf
Compare
Rebased. Several update-fixtures commit were removed before I generate a final update-fixtures commit based on current main. |
Let's either wait until tc39/proposal-explicit-resource-management#219 is merged, or implement it assuming that it will be accepted. |
TypeScript has merged the normative changes in microsoft/TypeScript#58624. I think we can implement the current spec as-is since tc39/proposal-explicit-resource-management#219 is not yet accepted. If that PR gets merged, we can always realign our implementation. |
Because the spec also requires that any sync errors thrown from |
Closing this PR in favor of #16537. |
This PR aligns our implementation to spec 7.5.6 GetDisposeMethod
There are two behaviour changes:
[Symbol.dispose]
method if the[Symbol.asyncDispose]
method isundefined
, previously we fallback when it isnull
[Symbol.dispose]
method.In this PR we also improve the error message when
[Symbol.asyncDispose]
method is not a function, previously we throw[Symbol.dispose]
is not a function, which could be very confusing.