Skip to content

Readd AsyncContext.Snapshot.wrap() helper#68

Merged
jridgewell merged 12 commits intomasterfrom
asynccontext-wrap
Feb 23, 2024
Merged

Readd AsyncContext.Snapshot.wrap() helper#68
jridgewell merged 12 commits intomasterfrom
asynccontext-wrap

Conversation

@jridgewell
Copy link
Member

Based on some feedback, it's useful to have a dedicated helper when only a single function needs to be wrapped. Having to create a Snapshot and return a new (...args) => snapshot.run(fn, ...args) is a little too cumbersome.

This should be very helpful while library support is still being implemented in the ecosystem. This allows consumers of unsupported libraries to quickly wrap callbacks they pass to the library.

@andreubotella
Copy link
Member

While looking at the test262 tests for Function.prototype.bind, I noticed that GetFunctionRealm is special-cased for bound function exotic objects. This patch would need to either modify that AO similarly, or add a [[Realm]] internal slot to AsyncContext wrapped function exotic objects.

@jridgewell
Copy link
Member Author

Done.

@jridgewell jridgewell changed the title Readd AsyncContext.wrap() helper Readd AsyncContext.Snapshot.wrap() helper Feb 12, 2024
constructor();

run<R>(fn: (...args: any[]) => R, ...args: any[]): R;
wrap<T, R>(fn: (this: T, ...args: any[]) => R): (this: T, ...args: any[]) => R;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little nitpicky, but would it be possible to put this directly onto AsyncContext? I imagine the most common usage will look something like

callback = AsyncContext?.Snapshot?.wrap(callback) ?? callback;

with the second ?. being defensive around some other non-conformant AsyncContext existing. We could do away with the extra optional chaining entirely if this were defined up a level.

Copy link
Member

Choose a reason for hiding this comment

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

Why would someone want to be defensive against a non-conformant AsyncContext implementation? I would imagine this more like,

callback = typeof AsyncContext === "object" ? AsyncContext.Snapshot.wrap(callback) : callback

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm OK with either position, but I definitely don't think a second ?. should be encouraged. I also kinda imagine using a utility function that would isolate this from most consumer code:

const wrap = (cb) => AsyncContext?.Snapshot.wrap(cb) ?? cb;


class Foo {
  test() {
    const cb = wrap(() => );
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly.

Copy link
Member

Choose a reason for hiding this comment

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

AsyncContext.wrap would be ambiguous since a function can also be wrapped with AsyncContext.Variable.prototype.run. I would find AsyncContext.Snapshot.wrap be more explicit that a function is wrapped with a snapshot.

Copy link
Member Author

Choose a reason for hiding this comment

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

AsyncContext.wrap would be ambiguous since a function can also be wrapped with AsyncContext.Variable.prototype.run

Re: #25

I would find AsyncContext.Snapshot.wrap be more explicit that a function is wrapped with a snapshot.

Let's keep as is then.

@legendecas
Copy link
Member

ecmarkup v18.2.0 is released but there are several unrelated errors to this PR if upgraded. Fixing in #72

@jridgewell jridgewell merged commit 733cb4f into master Feb 23, 2024
@jridgewell jridgewell deleted the asynccontext-wrap branch February 23, 2024 09:03
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.

7 participants