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

Reevaluate window.fetch each time HttpLink uses it, if not configured using options.fetch #8603

Merged
merged 3 commits into from
Aug 6, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 6, 2021

This should help with a variety of strategies for instrumenting window.fetch, as discussed in issue #7832.

Should help with a variety of strategies for instrumenting window.fetch,
as discussed in issue #7832.
@benjamn benjamn added this to the v3.4.x patch releases milestone Aug 6, 2021
@benjamn benjamn linked an issue Aug 6, 2021 that may be closed by this pull request
// #7832), or (if all else fails) the backupFetch function we saved when
// this module was first evaluated. This last option protects against the
// removal of window.fetch, which is unlikely but not impossible.
const currentFetch = preferredFetch || maybe(() => fetch) || backupFetch;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: There should no runtime difference between maybe(() => fetch) and backupFetch, as backupFetch is also maybe(() => fetch). In both cases the fetch variable is resolved during execution time of the maybe lambda. What is reason for the backupFetch fallback / the extra maybe(() => fetch)?

What is the reason for backupFetch here?

Suggested change
const currentFetch = preferredFetch || maybe(() => fetch) || backupFetch;
const currentFetch = preferredFetch || backupFetch;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bripkens A maybe(() => fetch) expression executes the () => fetch function immediately and returns the current result, or undefined if the function threw an exception. It's the shortest way I've found to evaluate possibly-undefined global variables without obtaining a reference to the global object, and TypeScript infers the return type reliably.

To my way of thinking, this means the maybe(() => fetch) expression we evaluate here could be different from backupFetch, which was evaluated when the module was first imported (possibly long ago). I think this difference allows window.fetch to be wrapped after the HttpLink is created (the goal of #7832, IIUC). If we switch to preferredFetch || backupFetch here, then we're stuck with one of those values for all time.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad, sorry @benjamn. I misunderstood what maybe is doing! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typeof fetch === "undefined" check might be clearer, but that’s a nit. I don’t like this maybe() function or relying on thrown ReferenceError in a callback, but I can tolerate it.

Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

@benjamn benjamn merged commit 28ef97c into main Aug 6, 2021
@benjamn benjamn deleted the issue-7832-HttpLink-later-fetch-binding branch August 6, 2021 20:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP link "fetcher strategy" circumvents common monitoring techniques
3 participants