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.

Sign up for GitHub

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

Jump to bottom

Avoid all sub-package imports from graphql npm package. #7185

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Avoid all sub-package imports from graphql npm package. #7185

merged 2 commits into from
Oct 19, 2020

Conversation

stoically
Copy link
Contributor

@stoically stoically commented Oct 17, 2020 •
edited
Loading

Fixes #7184 (and ardatan/graphql-tools#1790)

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@apollo-cla
Copy link

@stoically: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

stoically and others added 2 commits October 19, 2020 13:37
@stoically @benjamn
Import deps from graphql root
8cfd62f 
Fixes apollographql#7184
@benjamn
Avoid all sub-package imports from graphql package.
8a334f7 
Since we don't control the contents of this package, and nested
subdirectories within the graphql package do not have their own
package.json files with "main" and "module" entry points (like our
`@apollo/client/*` sub-packages do), it is unfortunately not safe to reach
into the package to import specific items, as demonstrated by apollographql#7184.

This could cause a bundle size regression for bundlers that do not perform
any tree-shaking, but it's the responsibility of the graphql package
maintainers to support selective sub-package imports if they want to.
Until that happens, @apollo/client must import from the graphql package in
the only supported way: directly from the top-level package.
@benjamn benjamn force-pushed the fix/graphql-webpack-esm branch from dcc940e to 8a334f7 Compare October 19, 2020 17:44
benjamn
benjamn approved these changes Oct 19, 2020
View reviewed changes
Copy link
Member

@benjamn benjamn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@stoically As I wrote in the commit message for 8a334f7, I think we can go even further here:

Since we don't control the contents of the graphql package, and nested subdirectories within the graphql package do not have their own package.json files with "main" and "module" entry points (like our @apollo/client/* sub-packages do), it is unfortunately not safe to reach into the package to import specific items, as demonstrated by #7184.

This could cause a bundle size regression for bundlers that do not perform any tree-shaking, but it's the responsibility of the graphql package maintainers to support selective sub-package imports, if they want to. Until that happens, @apollo/client must import from the graphql package in the only supported way: directly from the top-level package.

@benjamn benjamn added this to the Post 3.0 milestone Oct 19, 2020
@benjamn benjamn requested a review from hwillson October 19, 2020 17:49
@benjamn benjamn changed the title Import deps from graphql root in link/schema Avoid all sub-package imports from graphql npm package. Oct 19, 2020
@benjamn benjamn merged commit e145a86 into apollographql:main Oct 19, 2020
benjamn added a commit that referenced this pull request Oct 19, 2020
@benjamn
Mention PRs #7188 and #7185 in CHANGELOG.md.
ad2a542
This was referenced Oct 19, 2020
Closed
Merged
@stoically stoically deleted the fix/graphql-webpack-esm branch October 19, 2020 18:24
@AdrienLemaire
Copy link

AdrienLemaire commented Oct 26, 2020 •
edited
Loading

@benjamn @stoically when upgrading @apollo/client to v3.3.0-beta-14, I'm getting many errors related to grapqhl.

ERROR in ./node_modules/graphql/index.mjs 42:0-48:205
Can't reexport the named export 'BREAK' from non EcmaScript module (only default export is available)
 @ ./node_modules/@apollo/client/core/LocalState.js
 @ ./node_modules/@apollo/client/core/ApolloClient.js
 @ ./node_modules/@apollo/client/core/index.js
 @ ./node_modules/@apollo/client/main.cjs.js
 @ ./src/components/ApolloPersistentProvider.tsx
 @ ./src/index.tsx

ERROR in ./node_modules/graphql/index.mjs 60:0-97:42
Can't reexport the named export 'BreakingChangeType' from non EcmaScript module (only default export is available)
 @ ./node_modules/@apollo/client/core/LocalState.js
 @ ./node_modules/@apollo/client/core/ApolloClient.js
 @ ./node_modules/@apollo/client/core/index.js
 @ ./node_modules/@apollo/client/main.cjs.js
 @ ./src/components/ApolloPersistentProvider.tsx
 @ ./src/index.tsx

ERROR in ./node_modules/graphql/index.mjs 29:0-40:50
Can't reexport the named export 'DEFAULT_DEPRECATION_REASON' from non EcmaScript module (only default export is available)
 @ ./node_modules/@apollo/client/core/LocalState.js
 @ ./node_modules/@apollo/client/core/ApolloClient.js
 @ ./node_modules/@apollo/client/core/index.js
 @ ./node_modules/@apollo/client/main.cjs.js
 @ ./src/components/ApolloPersistentProvider.tsx
 @ ./src/index.tsx

...

After debugging the issue, I believe e145a86 is the reason for this issue.

No problem with v3.3.0-beta-13.

@stoically
Copy link
Contributor Author

@AdrienLemaire Might want to open a new issue with something to reproduce as well

@ivanprotasov
Copy link

@benjamn @stoically when upgrading @apollo/client to v3.3.0-beta-14, I'm getting many errors related to grapqhl.

ERROR in ./node_modules/graphql/index.mjs 42:0-48:205
Can't reexport the named export 'BREAK' from non EcmaScript module (only default export is available)
 @ ./node_modules/@apollo/client/core/LocalState.js
 @ ./node_modules/@apollo/client/core/ApolloClient.js
 @ ./node_modules/@apollo/client/core/index.js
 @ ./node_modules/@apollo/client/main.cjs.js
 @ ./src/components/ApolloPersistentProvider.tsx
 @ ./src/index.tsx

ERROR in ./node_modules/graphql/index.mjs 60:0-97:42
Can't reexport the named export 'BreakingChangeType' from non EcmaScript module (only default export is available)
 @ ./node_modules/@apollo/client/core/LocalState.js
 @ ./node_modules/@apollo/client/core/ApolloClient.js
 @ ./node_modules/@apollo/client/core/index.js
 @ ./node_modules/@apollo/client/main.cjs.js
 @ ./src/components/ApolloPersistentProvider.tsx
 @ ./src/index.tsx

ERROR in ./node_modules/graphql/index.mjs 29:0-40:50
Can't reexport the named export 'DEFAULT_DEPRECATION_REASON' from non EcmaScript module (only default export is available)
 @ ./node_modules/@apollo/client/core/LocalState.js
 @ ./node_modules/@apollo/client/core/ApolloClient.js
 @ ./node_modules/@apollo/client/core/index.js
 @ ./node_modules/@apollo/client/main.cjs.js
 @ ./src/components/ApolloPersistentProvider.tsx
 @ ./src/index.tsx

...

After debugging the issue, I believe e145a86 is the reason for this issue.

No problem with v3.3.0-beta-13.

We have the same issues on our prod project - @apollo/client/core/LocalState.js can't get 'BREAK' from 'graphql'. We have "graphql": "^15.4.0",

@guiaramos guiaramos mentioned this pull request Nov 20, 2020
Open
@benjamn
Copy link
Member

benjamn commented Nov 20, 2020

@AdrienLemaire @ivanprotasov Please see #7348 (comment), and, if that doesn't help, definitely open a new issue. Thanks!

@AdrienLemaire
Copy link

I confirm that adding ".mjs" to my webpack resolve: { extensions: [] } list fixed the issue, thanks!

@xpdmk
Copy link

xpdmk commented Mar 7, 2021

Looks like one subdirectory import from graphql has been missed:

apollo-client/src/link/http/createHttpLink.ts

Line 2 in 0728754

import { visit } from 'graphql/language/visitor';

@xpdmk xpdmk mentioned this pull request Mar 8, 2021
Closed
benjamn added a commit that referenced this pull request Mar 9, 2021
@benjamn
Avoid graphql/language/visitor import in createHttpLink.ts.
f20e114 
Closes #7807, reinforcing the intent of #7185.
This was referenced Mar 16, 2021
Closed
Closed
This was referenced Mar 16, 2021
Closed
Closed
Closed
Closed
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benjamn benjamn benjamn approved these changes

@hwillson hwillson Awaiting requested review from hwillson

Assignees
No one assigned
Labels
📦 bundle size 🎄 dependencies 🧞‍♂️ enhancement 🧩 implementation-detail 🛠 tooling 🙃 upstream
Projects
None yet
Milestone
Post 3.0
6 participants
@stoically @apollo-cla @AdrienLemaire @ivanprotasov @benjamn @xpdmk