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

Don't polyfill fetch for Node #5132

Merged
merged 1 commit into from
Sep 27, 2018
Merged

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Sep 27, 2018

Fixes #5124

Should we use isomorphic-fetch? Or leave this up to the user since using the Node test environment is "advanced"?

@Timer Timer added this to the 2.0.0 milestone Sep 27, 2018
@Timer Timer requested a review from gaearon September 27, 2018 13:19
@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2018

We used to always import it. Does that cause a problem? Is it broken in Node anyway? In that case I'd be fine with not supporting it there. On the other hand, shouldn't we expect jsdom to add it at some point? In that case it would make sense to polyfill it in Node. At least when jsdom is on.

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2018

It's currently broken in Node. In v1, the polyfills were a webpack entrypoint not a Jest setup file, so this didn't bite us until now.

@lixiaoyan
Copy link
Contributor

lixiaoyan commented Sep 27, 2018

FYI isomorphic-fetch is no longer under maintenance, universal-fetch is the active fork.

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2018

Last universal published 2 years ago, 3 for isomorphic.

What about cross-fetch? Seems to be published within 3 months.

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2018

Hmm, cross-fetch uses their own impl under the hood instead of whatwg-fetch, I don't like that.

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2018

It's currently broken in Node. In v1, the polyfills were a webpack entrypoint not a Jest setup file, so this didn't bite us until now.

OK

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2018

Wait. Polyfills were an entry point in Jest. How else could we get rAF there?

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2018

Oh, you're right... 🤔 I'll dig more into this, maybe it never worked and that many people left on the jsdom environment?

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2018

No idea.

My high level feedback is: if it was broken before I'd just remove it. If it kinda worked before, I'd leave it in the same state as it was. It should always be removed in non-jsdom environment.

I probably wouldn't add something like universal-fetch without more people asking for it. In tests you don't really want fetching to work. You usually want to mock it.

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2018

It should always be removed in non-jsdom environment.

Well, right now Jest can't even load when using the Node env and this at least allows tests to run again.

I assume people must've mocked out fetch before us trying to import it, but I think this is a better default.

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2018

Ah, found it. whatwg-fetch v2 would decorate global (self, fallback to this). v3 seems to have stricter protections to make sure it's only used in context of a browser.

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2018

Actually, this might be a bug: JakeChampion/fetch#657

But we still probably want the behavior of this PR by default.

@Timer Timer merged commit 328c312 into facebook:master Sep 27, 2018
@Timer Timer deleted the no-polyfill-node branch September 27, 2018 14:23
@gshilin
Copy link
Contributor

gshilin commented Nov 13, 2018

The same fix should be also merged to ie9.js and ie11.js

@Timer
Copy link
Contributor Author

Timer commented Nov 13, 2018

Feel free to send PR.

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
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.

5 participants