-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
While looking at the test262 tests for |
Done. |
AsyncContext.wrap()
helperAsyncContext.Snapshot.wrap()
helper
run<R>(fn: (...args: any[]) => R, ...args: any[]): R; | ||
wrap<T, R>(fn: (this: T, ...args: any[]) => R): (this: T, ...args: any[]) => R; |
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.
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.
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.
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
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'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(() => …);
}
}
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 don't feel strongly.
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.
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.
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.
AsyncContext.wrap
would be ambiguous since a function can also be wrapped withAsyncContext.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.
ecmarkup v18.2.0 is released but there are several unrelated errors to this PR if upgraded. Fixing in #72 |
Co-authored-by: Andreu Botella <[email protected]>
4ae5f62
to
1bdef9c
Compare
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.