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

Improve async dispose fallback #16409

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Improve async dispose fallback #16409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
43eed64
fallback to sync dispose on undefined asyncDispose
JLHwung Apr 4, 2024
61c830e
fix: do not await the promise returned from a sync dipose
JLHwung Apr 4, 2024
e57be3c
improve asyncDispose error
JLHwung Apr 4, 2024
8fb2a72
optimize helper size
JLHwung Apr 4, 2024
350dd2f
simplify error message
JLHwung Apr 5, 2024
947a930
update test cases
JLHwung Apr 5, 2024
c46ba2d
Update packages/babel-helpers/src/helpers/usingCtx.ts
JLHwung Apr 5, 2024
dcab815
tweak comment
JLHwung Apr 5, 2024
8869938
rename enums
JLHwung Apr 5, 2024
c3a7acf
update fixtures
JLHwung Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/babel-helpers/src/helpers-generated.ts
View file
Open in desktop
Expand Up @@ -188,10 +188,10 @@ export default Object.freeze({
"7.22.0",
'export default function _using(o,n,e){if(null==n)return n;if(Object(n)!==n)throw new TypeError("using declarations can only be used with objects, functions, null, or undefined.");if(e)var r=n[Symbol.asyncDispose||Symbol.for("Symbol.asyncDispose")];if(null==r&&(r=n[Symbol.dispose||Symbol.for("Symbol.dispose")]),"function"!=typeof r)throw new TypeError("Property [Symbol.dispose] is not a function.");return o.push({v:n,d:r,a:e}),n}',
),
// size: 922, gzip size: 481
// size: 922, gzip size: 496
usingCtx: helper(
"7.23.9",
'export default function _usingCtx(){var r="function"==typeof SuppressedError?SuppressedError:function(r,n){var e=Error();return e.name="SuppressedError",e.error=r,e.suppressed=n,e},n={},e=[];function using(r,n){if(null!=n){if(Object(n)!==n)throw new TypeError("using declarations can only be used with objects, functions, null, or undefined.");if(r)var o=n[Symbol.asyncDispose||Symbol.for("Symbol.asyncDispose")];if(null==o&&(o=n[Symbol.dispose||Symbol.for("Symbol.dispose")]),"function"!=typeof o)throw new TypeError("Property [Symbol.dispose] is not a function.");e.push({v:n,d:o,a:r})}else r&&e.push({d:n,a:r});return n}return{e:n,u:using.bind(null,!1),a:using.bind(null,!0),d:function(){var o=this.e;function next(){for(;r=e.pop();)try{var r,t=r.d&&r.d.call(r.v);if(r.a)return Promise.resolve(t).then(next,err)}catch(r){return err(r)}if(o!==n)throw o}function err(e){return o=o!==n?new r(e,o):e,next()}return next()}}}',
'export default function _usingCtx(){var r="function"==typeof SuppressedError?SuppressedError:function(r,e){var n=Error();return n.name="SuppressedError",n.error=r,n.suppressed=e,n},e={},n=[];function using(r,e){if(null!=e){if(Object(e)!==e)throw new TypeError("using declarations can only be used with objects, functions, null, or undefined.");if(r)var o=e[Symbol.asyncDispose||Symbol.for("Symbol.asyncDispose")],t=3;if(void 0===o&&(o=e[Symbol.dispose||Symbol.for("Symbol.dispose")],t&=2),"function"!=typeof o)throw new TypeError("Object is not disposable.");n.push({v:e,d:o,k:t})}else r&&n.push({d:e,k:2});return e}return{e:e,u:using.bind(null,!1),a:using.bind(null,!0),d:function(){var o=this.e;function next(){for(;r=n.pop();)try{var r,t=r.d&&r.d.call(r.v);if(r.k)return Promise.resolve(1&r.k&&t).then(next,err)}catch(r){return err(r)}if(o!==e)throw o}function err(n){return o=o!==e?new r(n,o):n,next()}return next()}}}',
),
// size: 1252, gzip size: 572
wrapRegExp: helper(
Expand Down
36 changes: 28 additions & 8 deletions packages/babel-helpers/src/helpers/usingCtx.ts
View file
Open in desktop
@@ -1,9 +1,24 @@
/* @minVersion 7.23.9 */

const enum DisposeFlag {
// set when the dispose function is a [Symbol.asyncDispose] method
ASYNC_DISPOSE = 1,
// alias of AWAIT_USING: set when the resource is an `await using` disposable
ASYNC_DISPOSE_MASK = 2,
}

const enum DisposeKind {
USING_DISPOSE = 0,
// AWAIT_FLAG
AWAIT_USING_DISPOSE = 2,
// Flag.AWAIT_USING | Flag.ASYNC_DISPOSE
AWAIT_USING_ASYNC_DISPOSE = 3,
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 renamed the enums so that hopefully two-year-later me will not get lost when reviewing the helper.

}

type Stack = {
v?: any;
d: null | undefined | (() => any);
a: boolean;
k: DisposeKind;
};

export default function _usingCtx() {
Expand All @@ -30,18 +45,20 @@ 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")],
Copy link
Contributor Author

@JLHwung JLHwung Apr 5, 2024 •
edited

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.

Copy link
Member

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. :)

Copy link
Contributor Author

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.

Copy link
Member

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.

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 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.

Copy link
Member

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]?

Copy link
Contributor Author

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.

Copy link
Member

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.

kind = DisposeKind.AWAIT_USING_ASYNC_DISPOSE;
}
if (dispose == null) {
if (dispose === undefined) {
dispose = value[Symbol.dispose || Symbol.for("Symbol.dispose")];
kind &= DisposeFlag.ASYNC_DISPOSE_MASK;
}
if (typeof dispose !== "function") {
throw new TypeError(`Property [Symbol.dispose] is not a function.`);
throw new TypeError("Object is not disposable.");
}
stack.push({ v: value, d: dispose, a: isAwait });
stack.push({ v: value, d: dispose, k: kind });
} else if (isAwait) {
// provide the nullish `value` as `d` for minification gain
stack.push({ d: value, a: isAwait });
stack.push({ d: value, k: DisposeKind.AWAIT_USING_DISPOSE });
}
return value;
}
Expand All @@ -62,8 +79,11 @@ export default function _usingCtx() {
try {
var resource,
disposalResult = resource.d && resource.d.call(resource.v);
if (resource.a) {
return Promise.resolve(disposalResult).then(next, err);
if (resource.k) {
return Promise.resolve(
// do not await the promise returned from the sync @@dispose
resource.k & DisposeFlag.ASYNC_DISPOSE && disposalResult,
).then(next, err);
}
} catch (e) {
return err(e);
Expand Down
27 changes: 24 additions & 3 deletions ...-proposal-explicit-resource-management/test/fixtures/exec-async/invalid-not-a-function.js
View file
Open in desktop
@@ -1,3 +1,24 @@
return expect(async function () {
await using foo = { [Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")]: 3 };
}()).rejects.toThrow(TypeError);
expect(
(async function () {
await using foo = {
[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")]: 3,
};
})()
).rejects.toThrow(TypeError);

expect(
(async function () {
await using foo = {
[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")]: 3,
};
})()
).rejects.toThrow("Object is not disposable.");

expect(
(async function () {
await using foo = {
[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")]: null,
[Symbol.dispose || Symbol.for("Symbol.dispose")]() {},
};
})()
).rejects.toThrow("Object is not disposable.");
35 changes: 35 additions & 0 deletions ...n-proposal-explicit-resource-management/test/fixtures/exec-async/sync-fallback-promise.js
View file
Open in desktop
@@ -0,0 +1,35 @@
return (async () => {
const log = [];

const promiseDispose = new Promise((resolve, _) => {
setTimeout(() => {
log.push("y dispose promise body");
resolve();
}, 0);
});

{
await using x = {
[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")]() {
log.push("x asyncDispose body");
},
};
await using y = {
[Symbol.dispose || Symbol.for("Symbol.dispose")]() {
log.push("y dispose body");
return promiseDispose;
},
};
}

log.push("body");

await promiseDispose;

expect(log).toEqual([
"y dispose body",
"x asyncDispose body",
"body",
"y dispose promise body",
]);
Copy link
Contributor Author

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)

})();
47 changes: 24 additions & 23 deletions packages/babel-runtime-corejs3/helpers/esm/usingCtx.js
View file
Open in desktop
Expand Up @@ -6,46 +6,47 @@ import _pushInstanceProperty from "core-js-pure/features/instance/push.js";
import _bindInstanceProperty from "core-js-pure/features/instance/bind.js";
import _Promise from "core-js-pure/features/promise/index.js";
export default function _usingCtx() {
var r = "function" == typeof _SuppressedError ? _SuppressedError : function (r, n) {
var e = Error();
return e.name = "SuppressedError", e.error = r, e.suppressed = n, e;
var r = "function" == typeof _SuppressedError ? _SuppressedError : function (r, e) {
var n = Error();
return n.name = "SuppressedError", n.error = r, n.suppressed = e, n;
},
n = {},
e = [];
function using(r, n) {
if (null != n) {
if (Object(n) !== n) throw new TypeError("using declarations can only be used with objects, functions, null, or undefined.");
if (r) var o = n[_Symbol$asyncDispose || _Symbol$for("Symbol.asyncDispose")];
if (null == o && (o = n[_Symbol$dispose || _Symbol$for("Symbol.dispose")]), "function" != typeof o) throw new TypeError("Property [Symbol.dispose] is not a function.");
_pushInstanceProperty(e).call(e, {
v: n,
e = {},
n = [];
function using(r, e) {
if (null != e) {
if (Object(e) !== e) throw new TypeError("using declarations can only be used with objects, functions, null, or undefined.");
if (r) var o = e[_Symbol$asyncDispose || _Symbol$for("Symbol.asyncDispose")],
t = 3;
if (void 0 === o && (o = e[_Symbol$dispose || _Symbol$for("Symbol.dispose")], t &= 2), "function" != typeof o) throw new TypeError("Object is not disposable.");
_pushInstanceProperty(n).call(n, {
v: e,
d: o,
a: r
k: t
});
} else r && _pushInstanceProperty(e).call(e, {
d: n,
a: r
} else r && _pushInstanceProperty(n).call(n, {
d: e,
k: 2
});
return n;
return e;
}
return {
e: n,
e: e,
u: _bindInstanceProperty(using).call(using, null, !1),
a: _bindInstanceProperty(using).call(using, null, !0),
d: function d() {
var o = this.e;
function next() {
for (; r = e.pop();) try {
for (; r = n.pop();) try {
var r,
t = r.d && r.d.call(r.v);
if (r.a) return _Promise.resolve(t).then(next, err);
if (r.k) return _Promise.resolve(1 & r.k && t).then(next, err);
} catch (r) {
return err(r);
}
if (o !== n) throw o;
if (o !== e) throw o;
}
function err(e) {
return o = o !== n ? new r(e, o) : e, next();
function err(n) {
return o = o !== e ? new r(n, o) : n, next();
}
return next();
}
Expand Down