Improve async dispose fallback by JLHwung · Pull Request #16409 · babel/babel · GitHub

Q	A
Fixed Issues?	See below
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This PR aligns our implementation to spec 7.5.6 GetDisposeMethod

1. If hint is async-dispose, then
  a. Let method be ? GetMethod(V, @@asyncDispose).
  b. If method is undefined, then
    i. Set method to ? GetMethod(V, @@dispose).
    ii. If method is not undefined, then
      1. Let closure be a new Abstract Closure with no parameters that captures method and performs the following steps when called:
        a. Let O be the this value.
        b. Perform ? Call(method, O).
        c. Return undefined.
      2. NOTE: This function is not observable to user code. It is used to ensure that a Promise returned from a synchronous @@dispose method will not be awaited.
      3. Return CreateBuiltinFunction(closure, 0, "", « »).
2. Else,
  a. Let method be ? GetMethod(V, @@dispose).
3. Return method.

There are two behaviour changes:

Only fallback to the [Symbol.dispose] method if the [Symbol.asyncDispose] method is undefined, previously we fallback when it is null
Do not await the promise returned from a sync [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.

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56800

In current main, the y dispose promise body is printed before x asyncDipose body (REPL)

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

Okay, how about "Object is not disposable.", since "dispose" is a verb.

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.

I prefer to unify them into something like disposal function is invalid. :)

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.

You are right! I changed it to invalid.

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.

Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose") always returns a symbol, so shouldn't we always look for value[symbol]?

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.

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:
This behavior seems interesting.

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 y is after body, even if it is in a sync dispose method. If there is no async disposable, the sync disposables will be disposed immediately, and the order becomes f body, y and body.

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>

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 dispose should be turned into a promise, a wrapper seems inevitable. I open a new PR #16537 without any changes from the disposable stack.

Closing this PR in favor of #16537.

JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Explicit Resource Management labels Apr 4, 2024

JLHwung commented Apr 4, 2024

View reviewed changes

Comment thread packages/babel-helpers/src/helpers/usingCtx.ts Outdated

liuxingbaoyu reviewed Apr 5, 2024

View reviewed changes

JLHwung commented Apr 5, 2024

View reviewed changes

liuxingbaoyu reviewed Apr 5, 2024

View reviewed changes

Comment thread packages/babel-helpers/src/helpers/usingCtx.ts Outdated

nicolo-ribaudo reviewed Apr 5, 2024

View reviewed changes

Comment thread packages/babel-helpers/src/helpers/usingCtx.ts Outdated

JLHwung commented Apr 5, 2024

View reviewed changes

JLHwung and others added 10 commits April 30, 2024 15:38

fallback to sync dispose on undefined asyncDispose

43eed64

fix: do not await the promise returned from a sync dipose

61c830e

improve asyncDispose error

e57be3c

Previously we throw "[Symbol.dispose] is not a function", which can be very confusing.

optimize helper size

8fb2a72

simplify error message

350dd2f

update test cases

947a930

Update packages/babel-helpers/src/helpers/usingCtx.ts

c46ba2d

Co-authored-by: Nicolò Ribaudo <hello@nicr.dev>

tweak comment

dcab815

rename enums

8869938

update fixtures

c3a7acf

JLHwung force-pushed the improve-asyncDispose-fallback branch from 0f4e40a to c3a7acf Compare April 30, 2024 19:39

liuxingbaoyu approved these changes Apr 30, 2024

View reviewed changes

JLHwung mentioned this pull request May 30, 2024

await using normative updates #16537

Merged

nicolo-ribaudo approved these changes Jun 20, 2024

View reviewed changes

JLHwung closed this Jun 20, 2024

github-actions Bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 20, 2024

github-actions Bot locked as resolved and limited conversation to collaborators Sep 20, 2024

                     if (isAwait) {
                       var dispose =
-                        value[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")];
+                          value[Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose")],

-                AWAIT_ASYNC = 3,
+                AWAIT_USING_DISPOSE = 2,
+                // Flag.AWAIT_USING | Flag.ASYNC_DISPOSE
+                AWAIT_USING_ASYNC_DISPOSE = 3,

+                  "x asyncDispose body",
+                  "body",
+                  "y dispose promise body",
+                ]);

                     }
                     if (typeof dispose !== "function") {
-                      throw new TypeError(`Property [Symbol.dispose] is not a function.`);
+                      throw new TypeError(

Improve async dispose fallback by JLHwung · Pull Request #16409 · babel/babel · GitHub

Uh oh!

Improve async dispose fallback - #16409

Improve async dispose fallback#16409JLHwung wants to merge 10 commits intobabel:mainbabel/babel:mainfrom JLHwung:improve-asyncDispose-fallbackJLHwung/babel:improve-asyncDispose-fallbackCopy head branch name to clipboard

Conversation

JLHwung commented Apr 4, 2024

Uh oh!

babel-bot commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxingbaoyu Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liuxingbaoyu commented Apr 5, 2024

Uh oh!

JLHwung commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung commented Apr 30, 2024

Uh oh!

nicolo-ribaudo commented May 5, 2024

Uh oh!

JLHwung commented May 30, 2024

Uh oh!

JLHwung commented May 30, 2024

Uh oh!

JLHwung commented Jun 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Improve async dispose fallback#16409
JLHwung wants to merge 10 commits into
babel:mainfrom
JLHwung:improve-asyncDispose-fallback

babel-bot commented Apr 4, 2024 •
edited
Loading

JLHwung Apr 5, 2024 •
edited
Loading

liuxingbaoyu Apr 5, 2024 •
edited
Loading