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

Re-exports from graphql-tag prevent Webpack from tree-shaking correctly #8217

Closed
nwalters512 opened this issue May 14, 2021 · 2 comments · Fixed by #8221
Closed

Re-exports from graphql-tag prevent Webpack from tree-shaking correctly #8217

nwalters512 opened this issue May 14, 2021 · 2 comments · Fixed by #8221

Comments

@nwalters512
Copy link

Intended outcome:

If my application never imports the gql tag from @apollo/client, no code from graphql-tag should end up in my bundle.

Actual outcome:

Even though my application never imports the gql tag from @apollo/client, a lot of code from graphql-tag (and graphql, transitively) ends up in my bundle.

How to reproduce the issue:

Here's a reproduction with a bog-standard Next.js app: https://github.com/nwalters512/graphql-tree-shaking-repro. Notice how nowhere in the application is gql ever imported - the only query (in src/pages/index.js) was precompiled to simulate the use of https://github.com/gajus/babel-plugin-graphql-tag.

Here's how to actually see that graphql-tag code is ending up in the bundle:

  • Run yarn to install dependencies
  • Build the app with yarn build
  • Look through the files in .next/static/chunks - one of them should contain the string Warning: fragment with name (on my machine the file was .next/static/chunks/425-4d21a67b7cb1dd316a5c.js). This corresponds to this line in graphql-tag - it's presence demonstrates that graphql-tag code is ending up in the bundle.

I believe this is caused by this re-exporting of values from graphql-tag in @apollo/client. You can demonstrate this root cause by editing node_modules/@apollo/client/core/index.js in my repro app as follows:

export { ApolloClient, mergeOptions, } from "./ApolloClient.js";
export { ObservableQuery, } from "./ObservableQuery.js";
export { NetworkStatus } from "./networkStatus.js";
export * from "./types.js";
export { isApolloError, ApolloError } from "../errors/index.js";
export { Cache, ApolloCache, InMemoryCache, MissingFieldError, defaultDataIdFromObject, makeVar, } from "../cache/index.js";
export * from "../cache/inmemory/types.js";
export * from "../link/core/index.js";
export * from "../link/http/index.js";
export { fromError, toPromise, fromPromise, throwServerError, } from "../link/utils/index.js";
export { Observable, isReference, makeReference, } from "../utilities/index.js";
import { setVerbosity } from "ts-invariant";
export { setVerbosity as setLogVerbosity };
setVerbosity("log");
import gql from 'graphql-tag';
-export var resetCaches = gql.resetCaches, disableFragmentWarnings = gql.disableFragmentWarnings, enableExperimentalFragmentVariables = gql.enableExperimentalFragmentVariables, disableExperimentalFragmentVariables = gql.disableExperimentalFragmentVariables;
+// export var resetCaches = gql.resetCaches, disableFragmentWarnings = gql.disableFragmentWarnings, enableExperimentalFragmentVariables = gql.enableExperimentalFragmentVariables, disableExperimentalFragmentVariables = gql.disableExperimentalFragmentVariables;
export { gql };
//# sourceMappingURL=index.js.map

Now, run rm -rf .next && yarn build to clear any caches and rebuild the app. Observe that the string Warning: fragment with name no longer appears in any client-side bundles - this shows that with the removal of that line from @apollo/client, code from graphql-tag is no longer included in the bundles.

I don't know how to actually fix this - the comment about the line in the source explains in great deal why that code is how it is. I also don't know exactly what on that line causes problems - my guess is that the re-exporting of destructured variables from another module throws off its tree shaking algorithm. This suspicion is bolstered by the output from Webpack's StatsWriterPlugin (which we use internally at NerdWallet, not in my sample app) which lists the reason that node_modules/graphql-tag/lib/index.js is included in the bundle as harmony side effect evaluation.

This is preventing us from realizing most benefits from babel-plugin-graphql-tag - even though we can still compile all queries at build time, we still end up shipping many kilobytes of dependencies from graphql and graphql-tag to clients, which negatively impacts performance, especially for low-end devices on slow networks.

Versions

  System:
    OS: macOS 10.15.7
  Binaries:
    Node: 14.15.4 - ~/.nvm/versions/node/v14.15.4/bin/node
    Yarn: 1.22.10 - ~/.nvm/versions/node/v14.15.4/bin/yarn
    npm: 6.14.10 - ~/.nvm/versions/node/v14.15.4/bin/npm
  Browsers:
    Chrome: 90.0.4430.212
    Edge: 90.0.818.56
    Firefox: 88.0
    Safari: 14.1
  npmPackages:
    @apollo/client: ^3.3.18 => 3.3.18 
@benjamn benjamn self-assigned this May 14, 2021
@benjamn benjamn added this to the Release 3.4 milestone May 14, 2021
benjamn added a commit that referenced this issue May 14, 2021
Seems to fix #8217, judging from the provided reproduction. Presumably
this is because using ECMAScript re-export syntax means we don't have to
import anything into the scope of the index.ts module, which makes
the whole situation easier for the tree-shaker to reason about.
@benjamn
Copy link
Member

benjamn commented May 14, 2021

@nwalters512 Thanks for the reproduction!

I can confirm the Warning: fragment with name string disappears from files in .next/static/chunks/ with #8221 applied.

Tree shaking can be fragile (and has multiple implementations with different limitations), but we control both of these packages (@apollo/client and graphql-tag), so with any luck this re-export relationship should keep working over time.

@nwalters512
Copy link
Author

Fantastic - thanks @benjamn!

Tree shaking can be fragile (and has multiple implementations with different limitations)

We are rapidly discovering this 😬 but we greatly appreciate that y'all are willing to support it as best you can. I think standard export { ... } from ... should indeed keep most tree-shaking implementations on the happy path.

benjamn added a commit that referenced this issue May 14, 2021
Fixes #8217, judging from the provided reproduction. Presumably
this is because using ECMAScript re-export syntax means we don't have to
import anything into the scope of the index.ts module, which makes
the whole situation easier for the tree-shaker to reason about.
@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants