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

Trim low-hanging fruit to reduce bundle sizes (part 1 of issue #4324). #4234

Merged
merged 11 commits into from
Jan 17, 2019

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Dec 13, 2018

Part of issue #4324, whose description previously appeared here.

This PR contains a handful of relatively minor bundle size improvements of the "low-hanging fruit" variety. Before making deeper and more systematic changes (like introducing a production bundle), we wanted to identify and remove (or simplify) unnecessary (or unnecessarily complicated) code.

@benjamn
Copy link
Member Author

benjamn commented Dec 20, 2018

I just changed the target branch for this PR to wip-reduce-bundle-size-reference-branch, which matches the current master branch. The plan is to avoid updating wip-reduce-bundle-size-reference-branch branch until after this PR is merged, so we won't lose track of our progress as we merge other bundle-size-reducing PRs into master. When this PR is ready to merge, we'll switch the target branch back to master and discard wip-reduce-bundle-size-reference-branch.

@danilobuerger
Copy link
Contributor

Transforming imports to be more specific using a compiler plugin, so that import { SpecificThing } from "apollo-client" becomes import { SpecificThing } from "apollo-client/lib/specific-thing".

This is not necessary. See tree shaking. Also discourage by some linters, for example: https://palantir.github.io/tslint/rules/no-submodule-imports/

@rosskevin
Copy link
Contributor

Agreed @danilobuerger

Related, I PR'd standardized bundles in #4261:

  1. main exploded cjs files and .d.ts
  2. module esm bundle
  3. browser umd bundle

The umd bundle is legacy and helps with putting your finger on the pulse of bundlesize (though not a foolproof indicator as it does not provide a tree shaken measurement).

Users concerned with tree shaking should ensure they are using the targeted module bundle.

@settings settings bot removed the in progress label Jan 2, 2019
@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

"Discouraged by some linters" is right up there with "frowned upon by some uncles" in terms of how much it should motivate our decisions. Remember that these transforms would be optional, and would happen during the build step. If you're linting the code generated by your build pipeline (rather than your source code), you may be missing the point of linting?

Not all bundlers do tree shaking, and not all tree-shaking bundlers do it reliably. Apollo is a set of libraries intended to be consumed in the widest possible variety of JavaScript environments, so I don't think we should be relying on the features of specific bundling tools.

@benjamn benjamn force-pushed the wip-reduce-bundle-size-reference-branch branch from a444e58 to d0abfea Compare January 2, 2019 19:29
@danilobuerger
Copy link
Contributor

@benjamn Your reply sounds rather rude to me, but I assume thats not on purpose. Now onto the content:

Making it easier to import specific parts of the Apollo Client packages, e.g. by supporting alternate entry points like apollo-client/core.

If I have to import like so:

import { Something } from "apollo-client/core"

I will have that in my source code. So now a linter that warns / errors on submodules will warn / error.

you may be missing the point of linting?

How am I missing the point of linting?

Not all bundlers do tree shaking, and not all tree-shaking bundlers do it reliably.

So what. Thats not really an argument. The implementer can choose a bundler that supports tree-shaking or not. Its his decision that generates the output.

@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

@danilobuerger I'm sorry for the way I phrased my previous comment, and I appreciate the correction.

I would like to clarify what I thought you were talking about. You're right that having to write

import { Something } from "apollo-client/core"

would require using submodule imports in your source code, but the idea that you quoted in your original comment was a different one:

  • Transforming imports to be more specific using a compiler plugin, so that import { SpecificThing } from "apollo-client" becomes import { SpecificThing } from "apollo-client/lib/specific-thing"

Since there's no /core at the end of the apollo-client module identifier here, there's nothing for a linter to complain about. The author of the source code automatically benefits from the additional specificity introduced during compilation, somewhat like @babel/transform-runtime inserting imports from @babel/runtime/helpers/.... If you did somehow have your linter configured to run on the generated code instead of your source code (strange as that would be), you'd have much bigger linting problems than this, and I'm sure you would agree that configuration was a mistake.

As for exposing modules like apollo-client/core, if you don't want to use them, you don't have to. Since their usage is not mandatory, and their existence does not prevent you from doing anything you could previously do, I'm not sure I see the objection?

So what. Thats not really an argument. The implementer can choose a bundler that supports tree-shaking or not. Its his decision that generates the output.

I think you may be over-focused on developers who have complete control over the software stack they have to work with. Speaking with the perspective of a library author, with customers who aren't able to use tree-shaking, I can assure you these (optional) features would have value, and they would complement whatever tree-shaking decisions the application developer may or may not be able to make.

@danilobuerger
Copy link
Contributor

Ah I see, just a misunderstanding. I should have quoted both examples in my initial reply, so its clear one is regarding tree shaking and the other regarding submodules.

For me it wasn't clear that importing from apollo-client (not any submodule) would still be possible.

So if I understand you correctly, the way to go would be:
a) Yes tree shaking, import apollo-client
b) No tree shaking, import apollo-client/core (or whatever submodule)

If thats the case, I agree.

But then I see a minor new problem and thats documentation. We should be careful to not confuse (especially new to apollo) developers with the different import options and follow one path through out.

@rosskevin
Copy link
Contributor

rosskevin commented Jan 2, 2019

But then I see a minor new problem and thats documentation. We should be careful to not confuse (especially new to apollo) developers with the different import options and follow one path through out.

Huge +1 on that. I learned a really painful tree shaking lesson when mixing cjs path imports with es index imports. Really flaky issue that there is no compatibility fix for. We should strongly encourage index imports to avoid interop problems and tree shaking optimization. Painful lesson details here: mui/material-ui-pickers#695 (comment)

@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

But then I see a minor new problem and thats documentation. We should be careful to not confuse (especially new to apollo) developers with the different import options and follow one path through out.

Absolutely agree. I'm imagining that all of our documentation examples should naively import everything from apollo-client, which is easier to learn than juggling multiple packages like apollo-boost and apollo-link-state.

If the application developer is using a bundler that does tree-shaking (essentially Webpack, Rollup, or Parcel), then they shouldn't have to worry about optimizing bundle sizes. Practically speaking, that means we should design these packages so that tree-shaking actually works reliably in those three bundlers, given the simple import ... from "apollo-client" style. I think the work that @rosskevin has been doing in #4261 will help a lot with this.

If the application developer is not using a tree-shaking bundler, and they are worried about their bundle sizes, it would be great to have an intermediate/advanced section of the docs explaining some other options (e.g., adding a Babel transform to rewrite import declarations, or switching to explicit sub-module imports). Recommending a different bundler is also valid advice, but may be more work than the other options.

@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

After reading @rosskevin's most recent comment, I'm reminded that explicit sub-module imports don't benefit from the module field in package.json, so you would have to do something like

import { SomeESMThing } from "apollo-client/esm/core"

instead of just apollo-client/core, which would require us to publish separate trees of modules under apollo-client/esm/... and apollo-client/cjs/.... Maybe we could make apollo-client/core a directory with its own package.json file with a different main and module field, but I'm not even sure every bundler would handle that the same way.

By comparison, if everyone sticks to writing

import { SomeThing } from "apollo-client"

then the behavior will depend on the main and module fields in package.json, and the application developer won't have to think about picking the right sub-module.

@rosskevin
Copy link
Contributor

If the application developer is not using a tree-shaking bundler, and they are worried about their bundle sizes, it would be great to have an intermediate/advanced section of the docs explaining some other options (e.g., adding a Babel transform

In what case would someone be concerned about bundle size, not use Webpack/Parcel but use babel?

I just want to make sure we aren't putting attention to a small fraction of users. If you are concerned about bundlesize and you are using advanced tools like babel, you really should be using a bundler - straight away.

What am I missing there?

@rosskevin
Copy link
Contributor

rosskevin commented Jan 2, 2019

By comparison, if everyone sticks to writing

import { SomeThing } from "apollo-client"
then the behavior will depend on the main and module fields in package.json, and the application developer won't have to worry about picking the right sub-module.

Exactly - this was a really painful lesson learned and I hope to help others avoid it. The only reason I have found to have exploded files is to match the .d.ts for downstream typescript users.

My strong recommendation at this point (I just realized cc: @JoviDeCroock) would actually be to only use this format (which my PR is close but not quite there as I have main pointed towards exploded):

  1. main -> bundle.cjs.js
  2. module -> bundle.esm.js
  3. types -> index.d.ts (exploded js files, preferably esm, next to .d.ts files).

Now the above with package.json pointing to only rollup bundles would guarantee a user would not encounter interop problems I mentioned, but then would not provide any path imports either. Given the hidden interop problems, this seems like the best set of artifacts for the majority of users. I'm not sure path imports are a good tradeoff given the hidden downsides and implications to tree shaking.

@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

@rosskevin You're right that the Venn diagram intersection is small, but one example is the Metro bundler used by React Native, which (last I checked, per facebook/metro#227) doesn't do tree-shaking. Arguably, tree shaking is a little less important in a React Native app, since you're not loading the code over a network, and tree-shaking is easier when you're building one big bundle, rather than doing incremental bundling on the fly, which I seem to recall being something Metro does? RN developers do use Babel plugins, though.

@bennypowers
Copy link
Contributor

bennypowers commented Jan 2, 2019

The easiest way to do this is to distribute es2018 in es modules. That would let app tools treeshake and transpile, and enable differential serving of small modern bundles to capable browsers aka >90 of your users (probably)

picture of three pieces of source code: on the left 4 lines of modern js, in the middle a transpiled version of same at around 20 lines, on the right a heavily transpiled version for 'production' targetting old browsers at around 50 lines

https://mobile.twitter.com/jamiebuilds/status/1072180184290217985

This would also enable more modern work flows using browser standards like ESM and web components.

Since transpiling and bundling should be largely app-level concerns, not library-level concerns, this would free developers to make lighter bundles and use innovative use cases.

See also #3047

@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

@bennypowers I wish it was that easy, but this library still has to be consumable by applications/bundlers that use require and only know about the main field in package.json, so unfortunately juggling both formats is the reality we have to live with.

@bennypowers
Copy link
Contributor

bennypowers commented Jan 2, 2019

That could be handled with "main": "cjs/index.js", no?

Also, rollup and webpack can handle es modules, so why not just bump a major version, while distributing a legacy bundle with all the babel cruft included for those that need it?

@rosskevin
Copy link
Contributor

That could be handled with "main": "cjs/index.js", no?

@bennypowers please see this comment as to why that is an interop problem #4234 (comment) and why I recommend rollup bundles for everything as detailed here #4234 (comment)

@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

@bennypowers I would recommend moving that discussion to #4261, where @rosskevin is hard at work figuring out the best way to support all environments/bundlers. Once you get up to speed on the tradeoffs he's considering, I think you'll agree it's not a simple problem. ✌️

@benjamn benjamn force-pushed the wip-reduce-bundle-size-reference-branch branch from d0abfea to 1da881c Compare January 2, 2019 22:07
@benjamn
Copy link
Member Author

benjamn commented Jan 2, 2019

Unrelated to the recent discussion, I'm beginning to realize that targeting wip-reduce-bundle-size-refrerence-branch with this PR (instead of master) is leading to a lot of unnecessary commit noise, especially since we've been merging master into this branch. On top of that, freezing the target branch doesn't seem to be helping with tracking total bundle size reductions, which was the original goal. In order to make this PR smaller and easier to review, I'm going to rebase it against master and change the target branch back to master (like all other PRs). This means the bundlesize check won't be entirely accurate, but we can still calculate the total reduction in other ways.

@benjamn benjamn changed the base branch from wip-reduce-bundle-size-reference-branch to master January 2, 2019 22:22
benjamn and others added 11 commits January 17, 2019 15:21
Part of #4224.

This change reduces the minified+gzip'd bundle size of apollo-client from
10385 bytes to 9771 bytes, a ~6% decrease, according to this command:

  npx terser -m -c < lib/bundle.umd.js | gzip | wc -c

As long as we're only building one bundle, and it has to work in the 15%
minority of browsers that do not natively support async/await, we should
avoid wasting precious bundle size on generator runtime code, especially
since we had only one await expression in the entire package (so the
runtime code was nowhere close to paying for itself).
If you look at the lib/bundle.umd.js files for the apollo-client packages,
you'll see multiple (re)declarations of TypeScript helpers like __extends,
__assign, and __rest. This happens because TypeScript injects those helper
function declarations into each module that uses them, so the declarations
appear multiple times when those compiled modules are concatenated.

Because the declarations share some identical code, gzip compression is
able to eliminate some of the repetition, but it would be better if each
helper appeared exactly once in each bundle.umd.js file.

One solution is to rely on a shared runtime library, which means putting
"importHelpers":true in your tsconfig.json file, and `npm install`ing the
tslib package, from which the helpers can be imported.

However, the tslib package contains all the helpers anyone could ever
need, which is far more than we actually need (about 2KB minified with
gzip, which is a lot). For this reason, we definitely do not want our
bundles to end up calling require("tslib") at any point.

Rollup to the rescue! By using rollup-plugin-node-resolve with { module:
true, only: ['tslib'] }, we can inline exactly the tslib exports that we
need, once, at the top of each bundle.umd.js file.

There will still be some overlap between the helpers declared by different
apollo-client packages, but at least now they will be exactly the same, so
gzip can work its magic with maximal efficiency.
Importing the whole GraphQL printer just for nicer errors isn't worth the
extra bundle size. JSON.stringify isn't as pretty, but provides all the
necessary information.
While knowing QueryDocumentKeys was helpful in theory, in practice we were
importing the entire graphql/language/visitor module unnecessarily, and
then had to tiptoe around the strange absence of primitive-valued keys
(for example, QueryDocumentKeys.StringValue is an empty array, when it
arguably should be ["value"]).
By privatizing the QueryManager, we give ourselves room to change its API
in drastic ways without affecting the ApolloClient API.
We're only tracking fetchQuery promises in order to cancel the in-flight
ones when clearStore is called, so we can get away with just storing the
reject functions, thereby saving bundle size.
It's nice to be able to assume that (sub)queries with equivalent structure
share the same object identities, but the cache will work without that
assumption, and enforcing that discipline is not free, both in terms of
runtime cost and in terms of bundle size.

In the future, the `graphql-tag` tag package should provide a version of
the `gql` template tag function that returns immutable structures that
share object identity where possible.

This commit alone saves 450ish bytes after minification and gzip!
This implementation has the following benefits:

- It collapses the QueryScheduler abstraction into the QueryManager (which
  was always ultimately responsible for managing the lifetime of polling
  timers), thus simplifying the relationship between the QueryManager and
  its ObservableQuery objects.

- It's about 100 bytes smaller than the previous implementation, after
  minification and gzip.

- It uses setTimeout rather than setInterval, so event loop starvation
  never leads to a rapid succession of setInterval catch-up calls.

- It guarantees at most one timeout will be pending for an arbitrary
  number of polling queries, rather than a separate timer for every
  distinct polling interval.

- Fewer independent timers means better batching behavior, usually.

- Though there may be a delay between the desired polling time for a given
  query and the actual polling time, the delay is never greater than the
  minimum polling interval across all queries, which changes dynamically
  as polling queries are started and stopped.
Though it's possible that someone might have been using flattenSelections
directly, the function signature seems awfully specific to the needs of
the former implementation of getDirectiveNames.
@benjamn
Copy link
Member Author

benjamn commented Jan 17, 2019

We've decided to go ahead and merge/release the initial progress we've made in this PR, even though we are still a long way from the goal of cutting bundle sizes in half. Let's move the ongoing bundle size discussion to issue #4324. Thanks to everyone for the productive conversation so far!

@benjamn benjamn changed the title [WIP] Reduce bundle sizes by whatever means necessary. Trim low-hanging fruit to reduce bundle sizes (part 1 of issue #4324). Jan 17, 2019
@benjamn benjamn merged commit 3510493 into master Jan 17, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants