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

feat: add support to forward args in context.with #1883

Merged

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Jan 28, 2021

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 and AsyncLocalStorage.

Allow to pass optional arguments to context.with() which are forwarded
to the passed function similar as in zone.js and AsyncLocalStorage.
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #1883 (326bce7) into main (cacbbdc) will decrease coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1883      +/-   ##
==========================================
- Coverage   92.72%   92.43%   -0.29%     
==========================================
  Files         172      155      -17     
  Lines        5963     5038     -925     
  Branches     1268     1072     -196     
==========================================
- Hits         5529     4657     -872     
+ Misses        434      381      -53     
Impacted Files Coverage Δ
...sync-hooks/src/AbstractAsyncHooksContextManager.ts 98.11% <ø> (ø)
packages/opentelemetry-api/src/api/context.ts 93.75% <100.00%> (+6.65%) ⬆️
...ontext-async-hooks/src/AsyncHooksContextManager.ts 100.00% <100.00%> (ø)
...async-hooks/src/AsyncLocalStorageContextManager.ts 100.00% <100.00%> (ø)
...entelemetry-context-base/src/NoopContextManager.ts 100.00% <100.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
...mentation-xml-http-request/src/enums/EventNames.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts
...rumentation/src/platform/browser/old/autoLoader.ts
... and 11 more

Copy link
Member

@dyladan dyladan left a 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));

@Flarna Flarna marked this pull request as draft January 29, 2021 23:59
Additionally allow to specify the receiver for the called function.

Improve typechecking by replacing unknown[] by varadic tuple types.
@Flarna
Copy link
Member Author

Flarna commented Feb 3, 2021

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.

@Flarna Flarna marked this pull request as ready for review February 3, 2021 18:22
Copy link
Member

@obecny obecny left a 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);
Copy link
Member

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 ?

Copy link
Member Author

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.

@dyladan dyladan merged commit 70a128f into open-telemetry:main Feb 8, 2021
@dyladan dyladan deleted the with-supports-args branch February 8, 2021 16:12
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

api.context.with() is lacking parameter for parameters to specified function
5 participants