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

[fetch] polyfill globally like all others #20963

Closed
wants to merge 2 commits into from

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jul 19, 2018

As @sqren mentioned in #20914 (comment) we are using three different fetch polyfills, and it's not totally clear which one is winning out. For some time import 'whatwg-fetch' has been included globally in the browser for the last 6 months or so, but wasn't compatible with jest tests running in node.js, which seems to be the reason people have been reaching for isomorphic-fetch. Rather than relying on the version of whatwg-fetch and node-fetch that isomorphic-fetch depends on, we can just rely on them directly and stub out the global fetch for jest tests in a jest setup module.

@spalger spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.4.0 labels Jul 19, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

👍

spalger pushed a commit to spalger/kibana that referenced this pull request Jul 19, 2018
As @sqren mentioned in elastic#20914 (comment) we are using three different fetch polyfills, and it's not totally clear which one is winning out. For some time `import 'whatwg-fetch'` has been included globally in the browser for the last 6 months or so, but wasn't compatible with jest tests running in node.js, which seems to be the reason people have been reaching for `isomorphic-fetch`. Rather than relying on the version of `whatwg-fetch` and `node-fetch` that `isomorphic-fetch` depends on, we can just rely on them directly and stub out the global fetch for jest tests in a jest setup module.
@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member

Looks like /kfetch.kfetch failing tests may be related to this PR

@spalger
Copy link
Contributor Author

spalger commented Jul 20, 2018

Yeah, I tried looking into it but it's unclear why the tests were passing before but not now. thinking it might be because of how old the node-fetch inside isomorphic-fetch was... but haven't spent time to really look into it

@azasypkin
Copy link
Member

azasypkin commented Aug 9, 2018

Hey @spalger , I'm removing review request from myself just to keep things on my Dashboard tidy, but feel free to request it once again if you still need one :)

@azasypkin azasypkin removed their request for review August 9, 2018 10:55
@spalger
Copy link
Contributor Author

spalger commented Sep 13, 2018

Might come back to this later.

@spalger spalger closed this Sep 13, 2018
@spalger spalger deleted the remove/isomorphic-fetch branch September 13, 2018 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants