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

Introduce apollo-fetch-polyfill package #30

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ctavan
Copy link
Contributor

@ctavan ctavan commented Aug 11, 2017

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:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes) -> Dropped dependency isomorphic-fetch in favor of cross-fetch (React Native compatible) #71 (comment)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • Add your name and email to the AUTHORS file (optional)
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion
  • 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.

@jbaxleyiii
Copy link
Contributor

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

@ctavan
Copy link
Contributor Author

ctavan commented Sep 4, 2017

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.

@ctavan
Copy link
Contributor Author

ctavan commented Sep 19, 2017

@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.

@giautm
Copy link

giautm commented Oct 2, 2017

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.
matthew-andrews/isomorphic-fetch#125
apollographql/apollo-link#75

@ctavan
Copy link
Contributor Author

ctavan commented Oct 4, 2017

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

@ctavan
Copy link
Contributor Author

ctavan commented Oct 5, 2017

@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.

@ctavan
Copy link
Contributor Author

ctavan commented Oct 6, 2017

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

@ctavan
Copy link
Contributor Author

ctavan commented Oct 7, 2017

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?

@ctavan
Copy link
Contributor Author

ctavan commented Oct 7, 2017

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

@evans
Copy link
Contributor

evans commented Dec 5, 2017

#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 evans closed this Dec 5, 2017
@evans evans reopened this Dec 6, 2017
@ctavan ctavan force-pushed the fetch-polyfilled-module branch 3 times, most recently from 105461a to 9693f44 Compare December 6, 2017 10:12
@ctavan
Copy link
Contributor Author

ctavan commented Dec 6, 2017

@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 ctavan mentioned this pull request Dec 6, 2017
@ctavan ctavan force-pushed the fetch-polyfilled-module branch 2 times, most recently from 7af6702 to 72e2cd6 Compare December 6, 2017 10:36
@evans
Copy link
Contributor

evans commented Dec 8, 2017

@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.

This leads to trouble with ts-jest not being able to reliably locate the
ts-jest/preprocessor.js module.
As opposed to mocha, jest runs each test file in a separate process.
Therefore we no longer need to worry about the polyfill, which is
included in one test, polluting the global namespace for the other test.
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.
@ctavan
Copy link
Contributor Author

ctavan commented Dec 8, 2017

Rebased again to resolve conflicts due to renovate bot updates.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants