Skip to content

Don't polyfill fetch for Node#5132

Merged
Timer merged 1 commit into
react:masterfrom
Timer:no-polyfill-node
Sep 27, 2018
Merged

Don't polyfill fetch for Node#5132
Timer merged 1 commit into
react:masterfrom
Timer:no-polyfill-node

Conversation

@Timer

@Timer Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

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

gaearon commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

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

Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

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

lixiaoyan commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

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

@Timer

Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

Last universal published 2 years ago, 3 for isomorphic.

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

@Timer

Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

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

@gaearon

gaearon commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

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

gaearon commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

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

@Timer

Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

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

@gaearon

gaearon commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

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

Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

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

Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

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

Timer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

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 react:master Sep 27, 2018
@Timer Timer deleted the no-polyfill-node branch September 27, 2018 14:23
@gshilin

gshilin commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

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

@Timer

Timer commented Nov 13, 2018

Copy link
Copy Markdown
Contributor Author

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