apollo-fetch

apollo-fetch copied to clipboard

Drop the global isomorphic-fetch polyfill from the main apollo-fetch package and provide a convenience package that simply re-exports apollo-fetch and includes the isomorphic-fetch polyfill.

TODO:

• [x] If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes) -> https://github.com/apollographql/apollo-fetch/pull/71#issuecomment-334810282
• [x] Make sure all of the significant new logic is covered by tests
• [x] Rebase your changes on master so that they can be merged easily
• [x] Make sure all tests and linter rules pass
• [X] Update CHANGELOG.md with your change
• [x] Add your name and email to the AUTHORS file (optional)
• [X] If this was a change that affects the external API, update the docs and post a link to the PR in the discussion
• [x] Find a better way to run the apollo-fetch test in isolation.

See #23 for discussion.

I had a bit of trouble with the apollo-fetch tests: I want to simulate an environment where fetch is present and therefore import isomorphic-fetch in the original tests. This has the bad side-effect, since mocha runs all tests in one process, that the no-polyfill.ts, where I want to explicitly test the behavior if the fetch API is not present, does not work as expected if it is executed together with the apollo-fetch.ts test, since the environment is already polyfilled… Any idea of how to improve this situation? Maybe switch to ava (which apollo-fetch-upload and apollo-fetch-polyfill already use) because it seems to have process isolation?

Currently the added test is also not contained in the coverage command.

Apart from that I'm curious about any additional feedback.

@ctavan sorry I missed this! I'll take a look this week!

Cool! I would really appreciate your feedback!

I just rebased this branch against the current upstream/master. As soon as you've found the time to look into this I'm more than happy to finish up this PR, especially with respect to the test-isolation-issue and potentially adding more documentation.

@jbaxleyiii now that Apollo Client 2.0 Beta is out, how do you want to proceed with the fetch-polyfill situation? I still believe that providing an apollo-fetch package by default (which requires fetch api to be present) and a convenience package apollo-fetch-polyfill (or apollo-fetch-polyfilled?) that comes bundled with a fetch polyfill would be the most flexible solution.

Hi Guys! Please roll-out this packages. Beta version of apollo-client can't work on react-native because stuck with isomorphic-fetch's bug. https://github.com/matthew-andrews/isomorphic-fetch/issues/125 https://github.com/apollographql/apollo-link/issues/75

@giautm I have updated this pull request to use cross-fetch instead of isomorphic-fetch. Would also love to see this merged.

@jbaxleyiii sorry for bumping this again. What holds us back from merging this?

I'd be more than happy to get this into a mergeable state. I just saw that apollo-client now uses jest for testing. Since jest also uses file-level isolation of tests it would allow us to solve the testing issue mentioned in the pull request description.

Any objections of porting the test-suite of apollo-fetch to jest? I'd be happy to do that to finally get this merged and be able to use apollo-client without polluting the global namespace.

I have update the test-suite to use jest, still have to figure out why lerna is not happy yet…

Hmm, since lerna hoists the ts-jest package to the root's node_modules, the test now fail due to this jest config:

    "transform": {
      ".(ts|tsx)": "<rootDir>/node_modules/ts-jest/preprocessor.js"
    },

In that case <rootDir> is the packages' root dir where ts-jest is no longer installed because it has been hoisted.

Any idea how to solve this?

I have set "hoist": false in the lerna config to resolve the ts-jest issue, which is also how apollo-client does it:

https://github.com/apollographql/apollo-client/blob/535710e1f5debf3c11fa8d4226326abeaeb97332/lerna.json#L5

#71 fixes the issue with React Native. From our past conversation, we may want to move to the polyfill for 1.0. Though it's looking like we'll deprecate the package.

Thank you @ctavan in any case for your PR!

@evans I understand you may want to discontinue to support this package. I still rebased this PR to the current master and I hope it should be good to merge.

I'll leave it up to you if you want to publish this as 1.0 or not.

@ctavan Thank you for rebasing!

I would like to have a lightweight client that is easy to use with middleware for auth. Whether it stays apollo-fetch or becomes more link based is something I have to think about.

Rebased again to resolve conflicts due to renovate bot updates.