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

Vendor whatwg-fetch #295

Closed
wants to merge 1 commit into from
Closed

Vendor whatwg-fetch #295

wants to merge 1 commit into from

Conversation

samselikoff
Copy link
Contributor

@samselikoff samselikoff commented Feb 27, 2020

Closes #293


Pretender uses the whatwg-fetch package in order to intercept fetch calls. However, Pretender relies implicitly on whatwg-fetch internals to get the fetch intercepting behavior working correctly.

This starting causing some problems in some systems where the whatwg-fetch package was swapped or removed altogether, since the package is indeed a polyfill and the system determined the behavior was present and thus the polyfill wasn't needed. However, because Pretender actually relied not only on the polyfill behavior but also internal implementation details, Pretender would break in these systems.

More details: The fetch implementation that whatwg-fetch provides actually delegates to window.XMLHTTPRequest under the hood. Since Pretender also monkey patches window.XMLHTTPRequest, it is able to intercept fetch calls equally as well. But it's only due to those internals – due to the fact that the whatwg-fetch polyfill happens to use XMLHTTPRequest under the hood. This is not part of whatwg-fetch's public API, even though it is unlikely the change.

Since Pretender has been relying on observable but undocumented behavior of whatwg-fetch, this commit vendors the package, ensuring that whatwg-fetch can no longer be treated as an external dependency and hoisted or replaced.

Pretender uses the `whatwg-fetch` package in order to intercept `fetch` calls. However, Pretender relies implicitly on `whatwg-fetch` internals to get the `fetch` intercepting behavior working correctly.

This starting causing some problems in some systems where the `whatwg-fetch` package was swapped or removed altogether, since the package is indeed a polyfill and the system determined the behavior was present and thus the polyfill wasn't needed. However, because Pretender actually relied not only on the polyfill behavior but also internal implementation details, Pretender would break in these systems.

More details: The `fetch` implementation that `whatwg-fetch` provides actually delegates to `window.XMLHTTPRequest` under the hood. Since Pretender _also_ monkey patches `window.XMLHTTPRequest`, it is able to intercept `fetch` calls equally as well. But it's only due to those internals – due to the fact that the `whatwg-fetch` polyfill happens to use XMLHTTPRequest under the hood. This is not part of `whatwg-fetch`'s public API, even though it is unlikely the change.

Since Pretender has been relying on observable but undocumented behavior of `whatwg-fetch`, this commit vendors the package, ensuring that `whatwg-fetch` can no longer be treated as an external dependency and hoisted or replaced.
@samselikoff
Copy link
Contributor Author

Once we merge this I will make a new PR that will supercede #269. It will be easy for us now to ensure Pretender is importable in node since we have whatwg-fetch vendored – we can just wrap the module in an if window check.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.9%) to 63.842% when pulling 5f15235 on vendor-whatwg-fetch into 6497376 on master.

@samselikoff
Copy link
Contributor Author

@xg-wang could I get your review on this?

@@ -1,5 +1,5 @@
import FakeXMLHttpRequest from 'fake-xml-http-request';
import * as FakeFetch from 'whatwg-fetch';
import * as FakeFetch from './whatwg-fetch';
Copy link
Member

Choose a reason for hiding this comment

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

You do not need to copy the whole file, external: Object.keys(pkg.dependencies), in rollup config is treating whatwg-fetch as external module, when you remove it from pkg.dependencies it will be imported by rollup.
You will still need whatwg-fetch in dev dependencies to let rollup find the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah. But it would make the next step easier since we can wrap it in a check to see if window exists. Otherwise we’ll need to do what I did in the other PR and bring in cross fetch, I think.

@xg-wang
Copy link
Member

xg-wang commented Mar 21, 2020

Closed by #296

@xg-wang xg-wang closed this Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Vendor whatwg-fetch
3 participants