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

fix(graphql): resolve core-js version collision #819

Closed
wants to merge 1 commit into from

Conversation

herschel666
Copy link
Contributor

@herschel666 herschel666 commented Mar 8, 2019

This PR fixes a current issue where it's not possible to start the local dev server due to errors.

Steps to reproduce

  • Create a graphql project: yarn create hops-app --template hops-template-graphql my-new-hops-project
  • Enter the project folder
  • Remove jest and jest-preset-hops dependencies: yarn remove jest jest-preset-hops
  • Start the app: yarn start
  • See the errors in console

The problem is a (partial) dependency tree like this with the latest version of apollo-server-express (2.4.8):

$ yarn list --pattern core-js
├─ apollo-env@0.3.4
│  └─ core-js@3.0.0-beta.13
└─ core-js@2.6.5

The respective yarn why-output for core-js is:

$ yarn why core-js

=> Found "core-js@2.6.5"
info Reasons this module exists
   - "hops#@untool#webpack#@babel#polyfill" depends on it
   - Hoisted from "hops#@untool#webpack#@babel#polyfill#core-js"
info Disk size without dependencies: "7.66MB"
info Disk size with unique dependencies: "7.66MB"
info Disk size with transitive dependencies: "7.66MB"
info Number of shared dependencies: 0
=> Found "apollo-env#core-js@3.0.0-beta.13"
info This module exists because "hops-graphql#apollo-server-express#apollo-server-core#@apollographql#apollo-tools#apollo-env" depends on it.
info Disk size without dependencies: "6.6MB"
info Disk size with unique dependencies: "6.6MB"
info Disk size with transitive dependencies: "6.6MB"
info Number of shared dependencies: 0


With the version range for apollo-server-express introduced by this PR, the (partial) dependency tree looks like this:

$ yarn list --pattern core-js
├─ @babel/polyfill@7.2.5
│  └─ core-js@2.6.5
└─ core-js@3.0.0-beta.13

And the respective yarn why-output for core-js is:

$ yarn why core-js

=> Found "core-js@3.0.0-beta.13"
info Reasons this module exists
   - "graphql-extensions#@apollographql#apollo-tools#apollo-env" depends on it
   - Hoisted from "graphql-extensions#@apollographql#apollo-tools#apollo-env#core-js"
info Disk size without dependencies: "6.6MB"
info Disk size with unique dependencies: "6.6MB"
info Disk size with transitive dependencies: "6.6MB"
info Number of shared dependencies: 0
=> Found "@babel/polyfill#core-js@2.6.5"
info This module exists because "hops#@untool#webpack#@babel#polyfill" depends on it.
info Disk size without dependencies: "7.66MB"
info Disk size with unique dependencies: "7.66MB"
info Disk size with transitive dependencies: "7.66MB"
info Number of shared dependencies: 0


The two different versions of core-js basically swap places in the tree, which fixes the issue with the local dev server… 🙃🤪😭

@herschel666
Copy link
Contributor Author

An alternative fix that works for me, is to install jest-preset-hops. 😵

@toomuchdesign
Copy link
Contributor

I still can't reproduce the error on my machine. 😕
It would be extremely helpful having the description of the steps which lead to the issue.

@herschel666
Copy link
Contributor Author

So @toomuchdesign and I did some more research on the topic. That's what we found:

This issue is related to the way Yarn hoists one of the two required core-js versions. Without jest & jest-preset-hops being required, there is exactly one package requiring core-js@2.6.5@babel/polyfill — and exactly one package requiring core-js@3.0.0-beta.13apollo-env.

Yarn then decides to hoist core-js@3.0.0-beta.13 to the root of the node_modules and install core-js@2.6.5 nested inside @babel/polyfill. Then during runtime @babel/polyfill fails to resolve its core-js-dependency correctly and ends up with the wrong version core-js@3.0.0-beta.13, which makes it fail epically.

If jest and/or jest-preset-hops are installed, too, there are several packages requiring version ^2 of core-js, leading Yarn to rather hoist that one up to the root and put core-js@3.0.0-beta.13 nested inside apollo-env. Then @babel/polyfill ends up with the correct version of core-js and everything is fine.

This problem does not occur when npm is used as a package manager…

Have a nice weekend everybody!

@toomuchdesign
Copy link
Contributor

toomuchdesign commented Mar 9, 2019

My latest explanation consists of @babel/polyfill injecting its polyfills imports statement taking for granted that the globally-hoisted version of core-js is the one @babel/polyfill itself relies on (v2).

The polyfilled files are then supposed to resolve their core-js imports from their location with the provided module resolution strategy.

But if:

  • someone introduces an additional version of core-js (in our case apollo-env) into the node modules tree
  • yarn decides that the this new version ofcore-js is the one supposed to be globally hoisted

... the polyfilled files will resolve to the wrong core-js version and not the one intended by @babel/polyfill.

This issue precisely describes our case.
@babel/preset-env docs warns about this issue and suggests a solution

@dmbch
Copy link
Contributor

dmbch commented Mar 10, 2019

Wow, this is interestingly bad... Does all this not imply we could add corejs@2 as a peerDependency to one of our packages (maybe hops-graphql) to make sure our users install it manually?

If I understand correctly, this would resolve the issue at hand - but it would be a 'breaking fix', would it not?

@robin-drexler
Copy link
Contributor

robin-drexler commented Mar 10, 2019

We had a similar issue happening already with some styled component related package. In this scenario the added corejs polyfills where in the wrong version, which caused some polyfills to not exist.

The fix was to install corejs in the project itself.

I think it's fair to add corejs as peer dependency, but maybe in the main hops package as having the correct corejs version has so many implication.

@toomuchdesign
Copy link
Contributor

toomuchdesign commented Mar 11, 2019

The most solid solution I can currently think of also consists of keeping a core-js@^2 dependency in the project itself (as long as @babel/polyfill relies on that version).

Since @untool/webpack is the library responsible for configuring babel and making use of useBuiltIns option I suppose we might consider declaring a peer dependency there.

@toomuchdesign
Copy link
Contributor

Discussion moved to @untool repo. See you there!
untool/untool#297

@herschel666 herschel666 deleted the fix-core-js-issue branch March 11, 2019 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants