-
Notifications
You must be signed in to change notification settings - Fork 805
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
feat: add support to forward args in context.with #1883
feat: add support to forward args in context.with #1883
Conversation
Allow to pass optional arguments to context.with() which are forwarded to the passed function similar as in zone.js and AsyncLocalStorage.
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.
Maybe in a follow up we can change the places where it is used. I think there are a lot of places that do something like:
context.with(ctx, () => somefn(arg1));
Additionally allow to specify the receiver for the called function. Improve typechecking by replacing unknown[] by varadic tuple types.
I updated this PR to allow also to specify the receiver (see #1870 (comment)). Besides that type checking should be better now. Would be nice if you could take a look again. |
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.
lgtm, one question
...args: A | ||
): ReturnType<F> { | ||
const cb = thisArg == null ? fn : fn.bind(thisArg); | ||
return this._asyncLocalStorage.run(context, cb as any, ...args); |
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.
shouldn't the cb be a type of F
?
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.
cb is F | OmitThisParameter<F>
but this._asyncLocalStorage.run()
requires (...args: any[]) => ReturnType<F>
.
Typescript tells then
Argument of type 'F | OmitThisParameter<F>' is not assignable to parameter of type '(...args: any[]) => ReturnType<F>'.
Type 'F' is not assignable to type '(...args: any[]) => ReturnType<F>'.
Type '(...args: A) => ReturnType<F>' is not assignable to type '(...args: any[]) => ReturnType<F>'.
Types of parameters 'args' and 'args' are incompatible.
Type 'any[]' is not assignable to type 'A'.
'any[]' is assignable to the constraint of type 'A', but 'A' could be instantiated with a different subtype of constraint 'unknown[]'.
I tried a little to avoid casting but I was not able to get typescript happy. Must likely because it assumes that our function gets called with unsupported arguments.
I could replace any
by as (...args: unknown[]) => ReturnType<F>
but in the end it just longer and harder to read.
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Which problem is this PR solving?
fixes #1870
Short description of the changes
Allow to pass receiver and arguments via
context.with()
to the passed function similar as in zone.js andAsyncLocalStorage
.