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

Allow late instrumentation of globalThis.fetch #57527

Closed
BenoitZugmeyer opened this issue Aug 26, 2024 · 4 comments
Closed

Allow late instrumentation of globalThis.fetch #57527

BenoitZugmeyer opened this issue Aug 26, 2024 · 4 comments
Assignees
Labels
area: common/http Issues related to HTTP and HTTP Client bug
Milestone

Comments

@BenoitZugmeyer
Copy link

Which @angular/* package(s) are relevant/related to the feature request?

common

Description

I am working on the Datadog Browser SDK, and we have customers having issues when using withFetch().

Our RUM SDK, like many other RUM solutions, is replacing the fetch global with an instrumented version (see our code, Sentry code, OpenTelemetry code). This works fine most of the time, but because Angular 18 is storing a reference of the fetch global internally, users willing to use withFetch() have to load and initialize our SDK before starting their Angular app, else Angular will use the native fetch instead of our instrumented version. This is not ideal as it forces the Angular app to wait for our SDK to be loaded and initialized before starting.

Proposed solution

Keep using the global fetch variable. This could be done by replacing

  private readonly fetchImpl =
    inject(FetchFactory, {optional: true})?.fetch ?? fetch.bind(globalThis);

with

  private readonly fetchImpl =
    inject(FetchFactory, {optional: true})?.fetch ?? (...args) => globalThis.fetch(...args)

here

Alternatives considered

Ask Angular 18 users to initialize third-party instrumentation libraries before starting their Angular app.

@angular angular deleted a comment Aug 26, 2024
@alxhub alxhub added area: common/http Issues related to HTTP and HTTP Client bug labels Aug 26, 2024
@ngbot ngbot bot added this to the needsTriage milestone Aug 26, 2024
@JeanMeche JeanMeche self-assigned this Aug 26, 2024
JeanMeche added a commit to JeanMeche/angular that referenced this issue Aug 26, 2024
Instead of using the reference that existing when `FetchBackend`  is setup.

fixes angular#57527
@JeanMeche
Copy link
Member

Hello,

Thank you for your investigation, the fixed you suggested is in review in #57531

JeanMeche added a commit to JeanMeche/angular that referenced this issue Aug 26, 2024
Instead of using the reference that existing when `FetchBackend`  is setup.

fixes angular#57527
JeanMeche added a commit to JeanMeche/angular that referenced this issue Aug 26, 2024
Instead of using the reference that existed when `FetchBackend` was setup.

fixes angular#57527
JeanMeche added a commit to JeanMeche/angular that referenced this issue Aug 27, 2024
Instead of using the reference that existing when `FetchBackend` is setup.

fixes angular#57527
alxhub pushed a commit that referenced this issue Aug 27, 2024
Instead of using the reference that existing when `FetchBackend` is setup.

fixes #57527

PR Close #57531
@alxhub alxhub closed this as completed in 21445a2 Aug 27, 2024
@alxhub alxhub reopened this Aug 28, 2024
@alxhub
Copy link
Member

alxhub commented Aug 28, 2024

Sadly this is still open as #57531 had to be reverted.

@JeanMeche
Copy link
Member

I'm investigating why the fix made our tests flaky.

JeanMeche added a commit to JeanMeche/angular that referenced this issue Aug 29, 2024
Instead of using the reference that existing when `FetchBackend` is setup.

fixes angular#57527
AndrewKushnir pushed a commit that referenced this issue Sep 3, 2024
Instead of using the reference that existing when `FetchBackend` is setup.

fixes #57527

PR Close #57531
mvdluit pushed a commit to mvdluit/angular that referenced this issue Sep 5, 2024
…7531)

Instead of using the reference that existing when `FetchBackend` is setup.

fixes angular#57527

PR Close angular#57531
mvdluit pushed a commit to mvdluit/angular that referenced this issue Sep 5, 2024
…7531)

Instead of using the reference that existing when `FetchBackend` is setup.

fixes angular#57527

PR Close angular#57531
and-oli pushed a commit to and-oli/angular that referenced this issue Sep 30, 2024
…7531)

Instead of using the reference that existing when `FetchBackend` is setup.

fixes angular#57527

PR Close angular#57531
and-oli pushed a commit to and-oli/angular that referenced this issue Sep 30, 2024
…7531)

Instead of using the reference that existing when `FetchBackend` is setup.

fixes angular#57527

PR Close angular#57531
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: common/http Issues related to HTTP and HTTP Client bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@BenoitZugmeyer @JeanMeche @alxhub and others