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

Rollup treeshaking and tslib #498

Merged
merged 6 commits into from
Mar 29, 2019
Merged

Rollup treeshaking and tslib #498

merged 6 commits into from
Mar 29, 2019

Conversation

patrickarlt
Copy link
Contributor

@patrickarlt patrickarlt commented Mar 28, 2019

In #490 @jgravois and I went deep into if it was worth creating another build in order to get tree shaking working for module aware bundlers like Parcel, webpack and Rollup. The main reason to get this working was to allow @jgravois to tree shake dependencies in Hub components which have complicated dependency trees like what he points out in #490 (comment)

in a Stencil.js web component, i want to:

import { followInitiative, unfollowInitiative } from "@esri/hub-initiatives";

the dependency graph for these methods includes:

  • request()
  • getUserUrl() - from @esri/arcgis-rest-auth
  • a couple utility methods from hub-initiatives

with es5 modules, the following ends up being included in my bundle.

  • request
  • auth
  • hub-initiatives - 4.3kb (of which i'm only using a small fraction)
  • items - a 1.3kb dependency of hub-initiatives that i'm not touching
  • groups - a 1kb dependency of hub-initiatives that i'm not touching
  • sharing - a 1.3kb dependency of hub-initiatives that i'm not touching
  • hub-domains a 300b dependency of hub-initiatives that i'm not touching
  • hub-sites a 600b dependency of hub-initiatives that i'm not touching
  • hub-common a 800b dependency of hub-initiatives that i'm not touching
  • ES2017 polyfills that evergreen browsers don't need

In the end we got webpack and Parcel working due to their support of a sideEffects flag in package.json. Rollup however has some issues stemming from our use of tslib and the importHelpers option in TypeScript.

While webpack and Parcel allow authors to mark their libraries an side-effect free with the sideEffects flag. Rollup take a more complicated algorithmic approach to figuring out what can be tree shaken and what cannot. Unfortunately that algorithm doesn't handle the code included in tslib very well which cases all code that users tslib to be marked as having side effects and not tree shakable.

As background TypeScript will automatically insert the tslib module when it needs common helpers like assign and extend if the importHelpers option is true. This option exists to avoid duplicating helper code across multiple modules, for example when both request and UserSession need assign they both get the single instance from tslib rather then their own 150 byte (minified + gzip) function in each module

This issue is actually pretty well known and has a few potential fixes:

This PR removes our use of tslib allow Rollup to tree shake our modules. What follows is a brief overview of what this change would do.


All modules would see a size increase in the esm modules. This is because almost every method uses the assign helper. This is ~150b per method after gzip. Some estimates are below:

  • request - gains ~450b
  • auth - gain ~300b
  • items - gain ~900b
  • groups - gain ~1kb

Also worth noting is that some methods still need tslib for some reason so those methods could never be tree shaken out.

HOWEVER there are also some significant savings here:

If you use only ApplicationSession from auth you save ~584b. Using just searchItems from items saves ~1kb. So an application using just ApplicationSession and searchItems would save ~1.5kb but gain ~750b total (~450b from request + 150b from UserSession and 150b searchItems) so there IS a net savings of ~750b.

Almost all of our usage of tslib could and thus most drawbacks of this by converting code that looked like:

requestOptions.params = {
  ...requestOptions.params,
  ...requestOptions.group
};

to

import objectAssign from ("object-assign"); // object assign ponyfill or similar util in common

requestOptions.params = objectAssign(
  {},
  requestOptions.params,
  requestOptions.group
);

This means TypeScript will no longer insert the code for its own assign helper. We could also add our own assign helper to the common utils and use it throughout to negate most of the tslib usage which would eliminate almost all of the penalties for taking this approach and allow us to better support Rollup users with tree shaking without waiting for a potential resolution in either rollup or tslib. If we implemented this across every method in our UserSession + searchItems example above we could potentially still save the 1.5kb and only bloat modules by ~150b (request still needs and extends helper).

@jgravois @tomwayson @dbouwman what do you think? Wait for this to be resolved upstream in either Rollup or tslib or implement a fix here and rely less on tslib?

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #498 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #498   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          95     95           
  Lines        1214   1214           
  Branches      225    225           
=====================================
  Hits         1214   1214

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bd5557...6ec7ca3. Read the comment docs.

@dbouwman
Copy link
Member

Let me start by bowing to your willingness to dig through this stuff!

Clearly, easiest thing would be to simply wait for upstream fixes to this. And while we're seeing reasonable percentage changes in package size, the packages are very small to begin with...

Sounds like Stenci.js uses rollup, so we'd have to do this to help @jgravois make tiny-pretty-things.

@jgravois can you try this and see if it works for you?

@patrickarlt
Copy link
Contributor Author

@jgravois I have no idea but for absolutely no reason at all this just started working in Rollup. Can you pull down this PR and do the following:

  1. Pull this branch
  2. npm run clean
  3. npm run bootstrap
  4. npm run build
  5. Build the rollup tree shaking example

and see if the output only has searchItems?

@jgravois
Copy link
Contributor

the output only has searchItems?

confirmed. 🚀

i've pushed up another commit with a little cleanup to ensure that the bootstrap command can lay down the dependencies your new demo needs.

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

i tested both your demo and in Stencil.js and confirmed that rollup's dead code elimination is kicking in and producing big savings.

thanks for all your help @patrickarlt!

@patrickarlt patrickarlt merged commit f9ad63d into master Mar 29, 2019
@jgravois jgravois deleted the rollup-tree-shaking branch March 29, 2019 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants