Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

whatwg-fetch -> isomorphic-fetch #4448

Closed
wants to merge 1 commit into from
Closed

Conversation

derhuerst
Copy link
Contributor

In order to make @parity/parity.js more portable, this PR removes the brittle & discouraged environment sniffing and uses isomorphic-fetch, which adds a cross-platform global fetch.

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Feb 6, 2017
@jacogr
Copy link
Contributor

jacogr commented Feb 6, 2017

Really not sure why we ended up with both whatwg-fetch and isomorpic-fetch since the original API implementations only used the latter - however it wasn't actually build-time included since the build used Rollup. (only a dependency in package.json)

Possibly due to merging with the codebases?

@ngotchac I remember us having a discussion at some point around this when the node-fetch/browser-fetch came up. Please do take a look.

This would obviously need to have a full binary build done to test if the served version works as expected when dapps request the libraries, i.e. cargo build --release --no-default-features --features ui

@ngotchac
Copy link
Contributor

ngotchac commented Feb 6, 2017

So here's the issue : we are building/transpiling our published libraries. Thus, on build time, Webpack has to know for each module which file to parse and include. To do that, it looks at the package.json file of each dependant modules. Let's say we use isomorphic-fetch : it will look at the browser field, than at the main field. For isomorphic-fetch, it will thus use the browser field, since it's available. In the end, the browser field of isomorphic-fetch just points to a file that requires whatwg-fetch. So for anyone using our library for a browser build, it will be fine, but not for those using it in a Node envrironment.

There is no real solution IMO, except those 2:

  • Have two builds per library, one for a browser and one for node, and explicit those 2 version in the published package.json file.
  • Let the user polyfill the fetch API if needed

@ngotchac
Copy link
Contributor

ngotchac commented Feb 6, 2017

I couldn't agree more on the fact that we currently do it the wrong way. To use it for the chrome extension, I had use some tricks in order not to polyfill the fetch API. That's why I personally prefer the second option. Plus, we should publish our source/ES6 code to NPM and point to the right file in package.json. Not everybody needs the compiled un-readable code

@derhuerst
Copy link
Contributor Author

derhuerst commented Feb 6, 2017

So here's the issue : we are building/transpiling our published libraries. […] For isomorphic-fetch, [webpack] will thus use the browser field, since it's available. […] So for anyone using our library for a browser build, it will be fine, but not for those using it in a Node envrironment.

Usually you publish libraries (to npm) in an unbundled, but transpired form. afaict this would fix the problem as well, right?

this has also the nice effect that stack traces are actually readable, as once can easily jump to the original file to see what's going on.

  • Let the user polyfill the fetch API if needed

If we don't take the approach described above (which i strongly recommend), 👍

@jacogr
Copy link
Contributor

jacogr commented Feb 6, 2017

Since we do a seperate library build -

  1. build-in to build for web to be served via /parity-utils/{inject,parity).js - this includes the browser fetch polyfill (hopefully what webpack includes)
  2. published version to have no built-in version, but rather isomorphic-fetch in package.json

@ngotchac
Copy link
Contributor

ngotchac commented Feb 6, 2017

Yes I think that would work (one bundled version via Webpack, one transpiled version that gets published)

@ngotchac ngotchac added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 7, 2017
@derhuerst
Copy link
Contributor Author

derhuerst commented Feb 7, 2017

I started to work on a setup that only transpires & publishes jsonrpc, parity.js, etherscan and shapeshift in the jr-simplify-release branch over at #4479.

@derhuerst derhuerst mentioned this pull request Feb 8, 2017
6 tasks
@derhuerst
Copy link
Contributor Author

Closing in favour of #4479. See de56706 specifically.

@derhuerst derhuerst closed this Feb 8, 2017
@derhuerst derhuerst deleted the jr-fetch-ponyfill branch February 8, 2017 17:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-inprogress ⏳ Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants