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

Issues with tree-shaking in v3 beta #5686

Closed
billyjanitsch opened this issue Dec 14, 2019 · 19 comments · Fixed by #8213
Closed

Issues with tree-shaking in v3 beta #5686

billyjanitsch opened this issue Dec 14, 2019 · 19 comments · Fixed by #8213

Comments

@billyjanitsch
Copy link

billyjanitsch commented Dec 14, 2019

Intended outcome:

I would expect these bundles to be roughly the same size:

import {Observable} from 'apollo-boost'
console.log(Observable)

and:

import {Observable} from '@apollo/client'
console.log(Observable)

Actual outcome:

The apollo-boost bundle is 7.6 KiB, whereas the @apollo/client bundle is 94.2 KiB.

How to reproduce the issue:

Compare this branch to this branch. To produce the bundle size measurements, check out either branch and run npm run build.

Versions

For the apollo-boost bundle:

  System:
    OS: macOS Mojave 10.14.6
  Binaries:
    Node: 12.13.1 - ~/.config/nvm/12.13.1/bin/node
    npm: 6.13.4 - ~/.config/nvm/12.13.1/bin/npm
  Browsers:
    Chrome: 79.0.3945.79
    Firefox: 69.0.2
    Safari: 13.0.4
  npmPackages:
    apollo-boost: ^0.4.7 => 0.4.7 

The @apollo/client one is the same except:

  npmPackages:
    @apollo/client: 3.0.0-beta.34 => 3.0.0-beta.34
@hwillson
Copy link
Member

@billyjanitsch any chance you could try things out with @apollo/client >= 3.0.0? AC3 should be tree-shakeable. We have examples apps to verify this here: https://github.com/apollographql/apollo-client/tree/master/examples/bundling

If you're still noticing issues though, definitely let us know. Thanks!

@billyjanitsch
Copy link
Author

Thanks for following up, @hwillson! I'll try it out this weekend and let you know.

@billyjanitsch
Copy link
Author

billyjanitsch commented Jul 17, 2020

@hwillson Unfortunately it's not fixed; it actually got even worse. 🙁

After upgrading @apollo/client from 3.0.0-beta.34 to 3.0.2, the bundle size increased from 94.2 KiB to 109 KiB.

@billyjanitsch
Copy link
Author

Here's a gist that may help with debugging. I took the bundle that gets built by my example repo at this SHA and ran it through Prettier to make it easier to read. It should help identify which parts of the client are included in the bundle beyond Observable.

@hwillson
Copy link
Member

hwillson commented Jul 17, 2020

Thanks for the additional details @billyjanitsch. Apollo Client 3 is tree-shakable, and working as expected. I'm not sure how the 7.6kB you mentioned for apollo-boost was calculated, but if you take a look at the following, you'll see they're pretty close:

AC3 adds more functionality in for sure, so it's a bit bigger. We're always working on reducing bundle sizes, and have changes coming that will help with this, but for now everything is working as expected.

As far as tree-shaking goes, if you want to validate it's working, you can npm i --save-dev unminified-webpack-plugin and update your webpack.config.js as:

const UnminifiedWebpackPlugin = require('unminified-webpack-plugin');

module.exports = {
  devtool: 'source-map',
  mode: 'production',
  plugins: [
    new UnminifiedWebpackPlugin()
  ]
}

Then after npm run build, if you open up the dist/main.nomin.js file and search for useQuery, you won't find anything. If you then adjust your index.js file as:

import { Observable, useQuery } from '@apollo/client';
console.log(Observable, useQuery)

then rebuild, open the dist/main.nomin.js file, and search for useQuery, you will see our useQuery React hook is now in the bundle.

@billyjanitsch
Copy link
Author

Thanks for the reply, @hwillson! Unfortunately, I think we're talking past each other a little.

I'm not sure how the 7.6kB you mentioned for apollo-boost was calculated

I linked to the Boost branch in the OP. It's exactly the same source code as the AC branch. The only difference is that one depends on Boost and the other depends on AC. Both sizes are computed by running npm run build.

I understand that the entire size of apollo-boost is comparable to the entire size of @apollo/client, which Bundlephobia verifies. My point isn't that AC is bigger than Boost; it's that AC fails to tree-shake a case that Boost was able to.

You can actually see this in your Bundlephobia links if you scroll down to the "Exports Analysis" sections. This section shows the size of the bundle if you only import each individual export from the library, rather than importing the whole thing. The reported size of the Observable export is 2.1kB for Boost and 21.1kB for AC. These are gzipped sizes so they're in the same ballpark as the sizes I reported (which are minified but not gzipped).

AC doesn't fail to tree-shake all cases; only some of them. That's why this isn't evident from the fixtures in the repo.

@billyjanitsch
Copy link
Author

billyjanitsch commented Jul 17, 2020

Another way to verify this is by adding unminified-webpack-plugin to the AC branch as you suggested. I also checked the resulting bundle into the repo so that I can link to it.

Sure enough, even though the code only imports Observable, the resulting bundle contains many other parts of AC, such as:

You're right that useQuery does get tree-shaken here, but the majority of AC doesn't.

@hwillson
Copy link
Member

Got it @billyjanitsch, thanks for clarifying. This is all definitely known. We balanced a few tradeoffs when restructuring AC3, and are currently favoring the majority of use cases when using @apollo/client out of the box. Most people want the cache, HttpLink, gql, etc. If you're using the default @apollo/client entry point, we're assuming you want ApolloClient. If you want ApolloClient, then you're going to need other supporting code that can't be removed, like the code you mentioned in #5686 (comment).

That being said, we do offer smaller entry points when people are looking for less. For example:

  • @apollo/client/core
  • @apollo/client/utilities
  • @apollo/client/cache
  • etc.

Your example of just wanting Observable is an interesting one. Observable comes from @apollo/client/utilities, so if you really just wanted it by itself, you would be better off importing it from @apollo/client/utilities directly. The being said though, it looks like Observable isn't currently exported from @apollo/client/utilities. It should be, so I'll fix that.

We're constantly looking for ways to improve all of this, so if you have any concrete suggestions on areas we can improve (that don't hinder the out of the box getting started @apollo/client experience), we're definitely all ears and would love the help. Thanks!

@benjamn
Copy link
Member

benjamn commented Jul 23, 2020

Heads up: we're not quite ready to publish @apollo/[email protected] yet, but I've published an @apollo/[email protected] release that I believe will resolve the Observable export issue, if you want to try that in the meantime. 🙏

@billyjanitsch
Copy link
Author

Hi @benjamn! Thanks for following up. Unfortunately, @apollo/[email protected] actually provides significantly worse tree-shaking than @apollo/[email protected] in this case. Compare the Bundlephobia results for the Observable export in 3.0.2 (21.1kB gzip) vs. 3.1.0-pre.0 (31.9kB gzip).

In fact, 3.1.0-pre.0 seems to provide worse tree-shaking results across the board. For example, the ApolloClient export has increased from 20.3kB to 27.3kB, and ApolloLink has increased from 5.7kB to 8.2.kB.

(Of course, the total size of @apollo/client has not changed significantly; this is just looking at individual exports.)

In any case, I appreciate the effort you've put in to fix this!


Your example of just wanting Observable is an interesting one.

@hwillson Thanks for your comment. To follow up on this point, Observable was just an example I chose to demonstrate the point. The way I actually ran into this was somewhat more complicated/realistic:

I had been precompiling all GraphQL queries in my app using babel-plugin-graphql-tag as recommended by the Apollo docs. This removed all imports of gql prior to bundling, and AC2 was therefore able to tree-shake away the dependencies on graphql-tag, etc. I was really happy with this arrangement since it also reduced the app initialization time (no longer had to parse N template strings on load).

When I upgraded to the AC3 beta, I noticed a massive increase in bundle size, and found that it was because this dependency, along with various other parts of AC I wasn't using, were no longer being tree-shaken.

I figured this specific scenario would be harder to explain in an issue, and I thought it might be mistaken for the incidental changes that AC3 made related to wrapping graphql-tag. So, instead, I found a single export that happened to demonstrate the same general point. Sorry if that ultimately made it harder to follow.

@benjamn
Copy link
Member

benjamn commented Jul 24, 2020

@billyjanitsch That may be related to #6687. I'll comment here when I've resolved that issue.

@billyjanitsch
Copy link
Author

I've been looking for some free time to dig into how AC's modules are structured to try to help out. From a brief look, though, I have two ideas:

  1. Webpack's sideEffects optimization is not recursive in the way that people tend to expect. If package a depends on package b, and a declares "sideEffects": false but b does not, Webpack will, in certain cases, bail out of pruning not just b's modules but some of a's modules as well, depending on how they import from b and what they do with those imports.

    So, even though AC declares that its modules do not contain side-effects, some of its dependencies don't make this declaration, so the module restructure in AC3 may have caused Webpack to bail out of tree-shaking optimizations in more cases.

  2. Webpack's module-pruning mechanism is significantly less aggressive when imports from CJS modules are involved. Even if they've imported using ESM syntax, Webpack still understands them to be CJS. This causes it to inject interop code and largely bail out of module concatenation which generally is required for the DCE portion of tree-shaking.

    Files like this one are a bit worrisome because they mix re-exports of actual ESM modules with pseudo-re-exports of CJS ones (e.g. zen-observable). The mere presence of the latter may force Webpack to include more of the graph and rely on DCE rather than module pruning which is much less effective.

If I'm right about either or both of these theories, this issue should be fixable without sacrificing the top-level exports of AC3 providing the out-of-the-box experience you're looking for. The fix would be a matter of shuffling around some of the modules, particularly the re-exports.

@redbar0n
Copy link

redbar0n commented Dec 19, 2020

@billyjanitsch Bundlephobia's Exports Analysis is bugged and not reliable: pastelsky/bundlephobia#364

@aquelehugo
Copy link

@benjamn any updates on this?

@kjenkins19
Copy link

@hwillson per your note above with the smaller entry points, the main issue I am having is with @apollo/client/core.
Graphql-tag is 8Kb gzipped. The export of gql is failing to tree shake properly.
I've updated to use the strategies noted at https://www.apollographql.com/docs/react/integrations/webpack/.
I can forcefully comment out the gql exports out and have my code compile (I can actually comment out 80% of the file and everything still works) since I'm only using a couple pieces of it.

There are a couple thoughts which may be of assistance:

  1. Remove all the of the import/export *. By default, if * is used on an import, webpack assumes side effects, therefore tree shaking will never fully be able to be utilized.
  2. There are dynamic imports inside of the core index file (typeof import("graphql-tag").resetCaches) which is probably also affecting it. If there is a way to abstract this to another file and exported so it only gets included when used, that may also be of help.

Just a couple theories, hope that helps.

@miqdadfwz
Copy link

Well I'm using the same webpack configuration (webpack v4), but ended up with significantly larger size comparing with former AC version.

Here is AC3 result using webpack bundle analyzer, it takes 39kb gzip along with graphql dependency. I'm afraid the tree shaking is partially not working here.
Screen Shot 2021-03-30 at 17 15 42

And here for AC2 with same chunks combination and other AC2 dependecies such as apollo-link-* and @apollo/react-hooks, it only takes 26kb.
Screen Shot 2021-03-30 at 17 15 35

@miqdadfwz
Copy link

@benjamn @hwillson I'm not a webpack expert but I have dug further as possible as I can, maybe this finding will help you addressing the issue. Before we continuing analyze the problem I have provide small repo for replication, and run pnpm run analyze:ui or pnpm run analyze:default

In this repo, the repo only have 4 important modules from AC3:

  1. ApolloClient which is located in src/index.js
  2. InMemoryCache you can find this in src/index.js
  3. ApolloProvider is put in src/App.js
  4. useQuery

In addition, you can see webpack-prod.config.js and .babelrc.js so you can figure this out how the build tools is configured.

pnpm run analyze:default will show you a large tree map representing the dependency contributes to this repo. We will ignore other dependencies as it will not be the main point of this write up, I have separated AC3 in the different chunk so we have better visibility for this. After you run the command, it will direct you to localhost: 8888.

c02ca908-daae-4a62-a3d2-ba729f37ce5d

As you can see at the above image, we almost import entire library of AC3 (~30kb gzipped). I have marked with red bordered rectangle as an example of module that we have not explicitly used but it is part of our large bundle size contributor. Let's narrow our scope to simplify the write-up, we will use useMutation or useSubscription as the primary object of the analysis.

b927ea80-cfbe-4035-a2c4-687f19f346b6

useSubscription and useMutation are the module of the unwanted outcome. Since it is related to the @apollo/react/hooks dependency, lets take a look at where the @apollo/react/hooks is imported, you can find this at src/PokemonList/index.js.

import { useQuery } from '@apollo/client/react/hooks';
import PokemonQuery from '../../queries/Pokemon/query.graphql';

const PokemonList = () => {
  const { data, loading } = useQuery(PokemonQuery, {
    variables: {
      first: 10
    }
  });
  
  ...

useQuery is the only module that we expect become our dependency from @apollo/client/hooks for the repo. We import useQuery module from @apollo/client/react/hooks as the react hooks related library gateway, which means at AC3 level, it re-export entire react hooks module for the consumer.

export * from "./useApolloClient.js";
export * from "./useLazyQuery.js";
export * from "./useMutation.js";
export * from "./useQuery.js";
export * from "./useSubscription.js";
export * from "./useReactiveVar.js";

With my current understanding, the code snippet above is not harm at all for tree shaking optimization and we expect webpack (and terser) will do remove unwanted react hooks module such as useMutation and useSubscription. The big question here is maybe at the some point of the code, webpack/terser fails trying to decide which part of blocks that actually safe to be removed, and it is kind of hard to debug since JavaScript is very dynamic in nature. But if you dig further to the @apollo/client/react/hooks project structure, seems it contains package.json which tells you that @apollo/client/react/hooks is "different" package from @apolloc/client. I’m afraid even though AC3 has defined sideEffect: false at the root level, it does not imply it will impact to its subdirectories which contains own package.json like hooks and ssr. If this hypothesis is correct, then maybe #5686 (comment) also true.

Webpack's sideEffects optimization is not recursive in the way that people tend to expect. If package a depends on package b, and a declares "sideEffects": false but b does not, Webpack will, in certain cases, bail out of pruning not just b's modules but some of a's modules as well, depending on how they import from b and what they do with those imports.

So, even though AC declares that its modules do not contain side-effects, some of its dependencies don't make this declaration, so the module restructure in AC3 may have caused Webpack to bail out of tree-shaking optimizations in more cases.

I’m trying to proof the concept here by adding sideEffect and set to false manually to the @apollo/client/react/hooks/package.json and the result satisfy my expectation:

d019afa6-2d5b-487b-bf16-488fc4864f3d

If you compare above image with the earlier given image, and with the consideration of what the repo only import useQuery, the tree shaking algorithm doing its job well. We don’t see any unnecessary hooks such as useMutation and useSupscription become part of repo dependency, because sideEffect: false tells the @apollo/client/react/hooks to skip statically whole unnecessary module and its subtrees. This is only small part of the case and maybe there is still many AC packages that need to be concerned. Hope this helps.

@benjamn benjamn modified the milestones: Post 3.0, Release 3.4 Apr 9, 2021
@benjamn benjamn self-assigned this Apr 9, 2021
@benjamn
Copy link
Member

benjamn commented Apr 9, 2021

Thanks for all the detailed info @miqdadfwz! We will dig into this before the Apollo Client v3.4 launch is finalized (though probably during the Release Candidate phase, since improving tree-shakeability should be relatively a transparent optimization, not a change in functionality).

@benjamn
Copy link
Member

benjamn commented May 13, 2021

Hello @miqdadfwz and others! We've released a change in @apollo/[email protected] to make sure every package.json file (including the ones in nested subdirectories) has "sideEffects": false, which should solve most/all of these problems. Please give that version a try when you have a chance.

@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants