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.

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

Readd AsyncContext.Snapshot.wrap() helper #68

Merged
merged 12 commits into from
Feb 23, 2024
Merged

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.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@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.

README.md Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@jridgewell jridgewell changed the title Readd AsyncContext.wrap() helper Readd AsyncContext.Snapshot.wrap() helper Feb 12, 2024
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
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.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@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
5 checks passed
@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