chore: provide esm bundle #2677

JoviDeCroock · 2018-12-18T21:54:30Z

So I optimised the build process so PURE_CALLS etc are annotated,.... This in turn shaves off 3KB.
I've seen this babel-jest issue before and I'm searching on it, if anyone passes here and knows more about it feel free to correct it on my branch.

I'll gladly make work of this in apollo-client aswell if this one is according to your wishes.

Some other things i'll be looking into after this is probably if closure compiler is a viable idea

JoviDeCroock · 2018-12-20T22:28:34Z

Sorry but i'm not really finding the issue... I know it's because off some missmatch in config but yes... Any help appreciated...

package.json

rollup.config.js

benjamn · 2018-12-21T17:16:16Z

My concern about this PR is that it seems to be optimizing the way third-party npm packages are bundled into the dist/bundlesize.js bundle, but not actually reducing the compressed size of lib/react-apollo.umd.js or lib/react-apollo.browser.umd.js. In other words, the techniques you're using here might be a good recommendation for developers who are bundling an application that uses react-apollo (along with other packages), but I'm not sure we can really claim we've reduced the footprint of react-apollo itself. Am I understanding this correctly?

babel.config.js

JoviDeCroock · 2018-12-21T17:18:33Z

My concern about this PR is that it seems to be optimizing the way third-party npm packages are bundled into the dist/bundlesize.js bundle, but not actually reducing the compressed size of lib/react-apollo.umd.js or lib/react-apollo.browser.umd.js. In other words, the techniques you're using here might be a good recommendation for developers who are bundling an application that uses react-apollo (along with other packages), but I'm not sure we can really claim we've reduced the footprint of react-apollo itself. Am I understanding this correctly?

Well in production now the invariant messages are cut away and while bundling pure functions won't be repeated. Allthough what you are stating is partially true it's also safe to assume this reduces the react-apollo bundle size.

Feel free to counter this point. Because I can't really see why this is not improving this package.

my concerns addressed

rosskevin · 2018-12-21T17:25:23Z

If this is already done downstream (in app bundling) then we shouldn't merge it, but if we cannot count on it downstream then I agree it should be considered. As I mentioned in #2661, we should not necessarily conflate our rollup bundle sizes with ultimate app bundled sizes when using a modern tool like webpack or parcel.

JoviDeCroock · 2018-12-21T17:28:41Z

So all together: I can remove DEV and it would only increase like 150B (why i think it only targets our invariants but i'm not sure).

If this PR gets approved, should I do the same in the normal apollo-client? :)

benjamn · 2018-12-21T17:51:30Z

@JoviDeCroock Can you explain more about how the /*#__PURE__*/ comments work? What part of the bundled code are we hoping to eliminate? What bundling tools will benefit from those annotations? Are there any recommendations that we should be making to developers?

From what I can tell, this PR slightly increases the sizes of lib/react-apollo.umd.js (6257B to 6333B, with minification+gzip) and lib/react-apollo.browser.umd.js (6231B to 6307B). What's your understanding of why that would be happening? At first glance, it seems important because lib/react-apollo.umd.js is the main field in package.json, whereas dist/bundlesize.js is just a CircleCI build artifact. But maybe these changes mean that some npm dependencies could be dropped, so the (small) increase is offset by other savings?

JoviDeCroock · 2018-12-21T17:59:28Z

@benjamn what it does is it function at the top level when they're pure. This in turns helps tools like uglify do a way better job at dead code elimination. This implies a bit more code in our umd bundles (since it has some annotations but after passing through terser/uglify/... Potentially way less code. These top level calls will also be recognised in assignment contexts and thus could be reused.

What i don't understand is why after minification it's bigger since when i tried to minify + gzip our umd bundles (before PR) i had 10KB... So might just be me overdoing this. So yes can close if need be

This sais the current exported bundle is 8.7 after minification and gzip - https://bundlephobia.com/result?p=react-apollo@2.3.3
You state that the build artifact is irrelevant then or?

JoviDeCroock · 2018-12-27T07:26:07Z

After all the feedback, I'll probably close this one in favor of just making a normal ES bundle. Note that one of the few things you can do then to reduce bundle size in RA is to cut in dependencies because they take up 50% of RA's bundle.

rosskevin · 2018-12-27T14:03:19Z

Note that one of the few things you can do then to reduce bundle size in RA is to cut in dependencies because they take up 50% of RA's bundle.

But again, umd bundle size matters very little to an app's bundled usage with tree shaking of an es module. The umd is a data point, in some simple libraries a perfect measure, but be aware for example that I don't use the reexported compose and therefore pay no price for the size of it. I just want to make sure that are on the same page.

The first order of business to to make sure that our bundle and our dependencies bundles are tree shakable, then go from there.

Umd size can be a vanity metric, let's make sure we don't mistake it as the only indicator of a healthy bundle.

JoviDeCroock · 2018-12-27T14:08:31Z

Yes but things like DEV Are meant for library authors so the pure functions get annotated for things like your terser plugin.

That being said you are right that an ES Bundle will probably decrease the bundle size for the people using react-apollo.

rosskevin · 2018-12-27T14:21:56Z

Definitely agree on that.

rosskevin · 2018-12-27T14:24:36Z

BTW in my apollo-client PR, I noticed an analyze project/folder for webpack. Maybe we should consider the same here as a tool for this process.

JoviDeCroock · 2019-01-03T14:04:50Z

Okay, I've put some thought into this and I would fully DROP the build artifact and use the build artifact in a more optimal manner. Let bundleSize itself figure out the gzipped size and validate the ES, CJS and UMD output itself.

This way discussions like these can be avoided? If this gets approved by @benjamn @hwillson @rosskevin I'll gladly implement it, this way we can cleanly show the bundleSizes instead of this umd artifact.

I think this will also make a huge benefit for apollo-client.

rosskevin · 2019-01-03T14:31:16Z

I think that is a good idea. Let's PR that here and review it, then I'll take those changes to the apollo-client monorepo PR.

rosskevin · 2019-01-03T17:36:15Z

https://github.com/TrySound/rollup-plugin-size-snapshot looks nicer for sizing.

JoviDeCroock · 2019-01-03T17:51:45Z

https://github.com/TrySound/rollup-plugin-size-snapshot looks nicer for sizing.

I think that can't be integrated in our build process (as in the PR checks etc). Maybe it can i'll look into it

JoviDeCroock · 2019-01-04T07:06:36Z

I have drastically altered this proposal in favor of just providing an esm bundle, cjs bundle coming tonight :)

rosskevin

Ok, so this appears to be somewhat similar to what I saw in apollo-client and seems strange to me. We use tsc -> lib then bundle from lib, when we should just be bundling directly from index.ts with rollup (no intermediary tsc necessary). I don't believe the input to any bundle should be the output of another build step, we should build from source directly in case the configs change (and they need to as detailed below).

^^^is also important because it was decided in apollographql/apollo-client#4261 (comment) to have the following artifacts:

main -> exploded js and .d.ts - this will require changes to tsconfig for the tsc step
No cjs bundle, no umd bundle (provided as an artifact, we should ignore these in npmignore)
module -> esm bundle
no umd build referenced as an artifact (and no browser property in package.json)

I think this staying in-line with the decisions for apollo-client will provide for consistency. You can check that PR for tsconfig and rollup configurations.

JoviDeCroock · 2019-01-04T20:16:40Z

Last comment I saw was bundles cjs, using tsc beforehand results in the same as rollup-tsc just 4 seperate times vs once. Eh nevermind, i’ll wait until it’s all cleared up or something

rosskevin · 2019-01-04T20:33:21Z

We are removing the cjs bundle from rollup, there is one tsc run for exploded cjs. This is clear, no need to wait.

We can also remove totally the umd - it would only be useful for a bundlesize and is not exposed externally.

JoviDeCroock · 2019-01-05T10:36:44Z

I have made one more attempt, it just feels like the requirement changes a lot and if this changes. I'll probably wait until the apollo-client one gets merged so we have a unary steering direction.

Made a seperate tsconfig.cjs.json since rollup can't compile commonjs.

Also safe to say that as what I wanted to avoid by using tsc once or twice is now what I feared, compiling takes ages. Two-Five minutes total.

rosskevin · 2019-01-07T15:21:44Z

I think this looks good.

Timewise there are a few things we can do to improve it:

the price should only paid on bundling for release, so postcompile should really be prepublishOnly - that kills the rollup step entirely for the dev workflow - see https://docs.npmjs.com/misc/scripts
Even in the rollup step, I think we may be able to kill the umd builds later

Nonetheless, I do recommend postcompile -> prepublishOnly which I think is an improvement intended by npm that we haven't been using in the past.

JoviDeCroock · 2019-01-07T16:24:38Z

I think this looks good.

Timewise there are a few things we can do to improve it:

the price should only paid on bundling for release, so postcompile should really be prepublishOnly - that kills the rollup step entirely for the dev workflow - see https://docs.npmjs.com/misc/scripts

Even in the rollup step, I think we may be able to kill the umd builds later

Nonetheless, I do recommend postcompile -> prepublishOnly which I think is an improvement intended by npm that we haven't been using in the past.

Do you want me to change that now or?

rosskevin · 2019-01-07T16:26:11Z

Sure that would be great!

* chore: optimize build time * fix: remove comments * tests: revert transforms * fix tests * remove babel key from pkg.json * chore: apply pr remarks * chore: undo all and add esm bundle with potential todo * remove unused babel plugins * cjs * some optimizations * add question * chore: follow pr * ignore rpt_cache

Similar in spirit to apollographql/apollo-link#959 This inlining was first introduced in PR #2661 with the following commit: de2b5fc At the time, inlining made sense because TypeScript was injecting copies of the __extends, __rest, etc. helpers into every module that used them. Depending on the tslib package seemed undesirable because the available bundle size measurement tools (e.g. bundlephobia.com) mistakenly counted the entire tslib package against react-apollo, without acknowledging the possibility of sharing that package between multiple Apollo packages. It seemed safer to inline only the helpers we needed at the top of lib/react-apollo.esm.js. Now that we have a more holistic way to measure bundle sizes (#2839), and react-apollo works better with tree-shaking tools (#2659, #2661, #2677), we know that overall application bundle sizes benefit from sharing a single copy of the tslib helper package, even if no tree-shaking is happening. Of course, with tree-shaking, that one copy of the tslib package can be shrunk to contain just the helpers that are actually used.

chore: optimize build time

1a11e1e

JoviDeCroock requested review from benjamn and hwillson as code owners December 18, 2018 21:54

JoviDeCroock changed the title ~~chore: optimize build time~~ chore: optimize build output (bundle size -4.5KB) Dec 18, 2018

fix: remove comments

146810d

JoviDeCroock added 3 commits December 21, 2018 17:10

tests: revert transforms

96fae83

fix tests

8a6c53e

remove babel key from pkg.json

6e24cc9

rosskevin suggested changes Dec 21, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

rollup.config.js Outdated Show resolved Hide resolved

rollup.config.js Outdated Show resolved Hide resolved

chore: apply pr remarks

a9fd2ca

rosskevin mentioned this pull request Dec 21, 2018

[WIP] Reduce bundle size. #2661

Merged

rosskevin previously requested changes Dec 21, 2018

View reviewed changes

babel.config.js Outdated Show resolved Hide resolved

chore: undo all and add esm bundle with potential todo

a73b110

JoviDeCroock changed the title ~~chore: optimize build output (bundle size -4.5KB)~~ chore: provide esm bundle Jan 4, 2019

JoviDeCroock added 4 commits January 4, 2019 08:08

remove unused babel plugins

d314f2c

cjs

4e4d55f

some optimizations

1525445

add question

7956f4a

rosskevin suggested changes Jan 4, 2019

View reviewed changes

JoviDeCroock closed this Jan 4, 2019

JoviDeCroock added 2 commits January 5, 2019 11:32

chore: follow pr

f65cd81

ignore rpt_cache

56b3957

JoviDeCroock reopened this Jan 5, 2019

rosskevin approved these changes Jan 7, 2019

View reviewed changes

rosskevin merged commit 63a83ab into apollographql:wip-reduce-bundle-size Jan 8, 2019

This was referenced Jan 15, 2019

Exporting as ES Module Syntax (ESM) apollographql/apollo-feature-requests#83

Closed

chore: provide CJS and ESM bundle apollographql/graphql-tag#234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: provide esm bundle #2677

chore: provide esm bundle #2677

JoviDeCroock commented Dec 18, 2018 •

edited

Loading

JoviDeCroock commented Dec 20, 2018

benjamn commented Dec 21, 2018

JoviDeCroock commented Dec 21, 2018

rosskevin commented Dec 21, 2018

JoviDeCroock commented Dec 21, 2018 •

edited

Loading

benjamn commented Dec 21, 2018 •

edited

Loading

JoviDeCroock commented Dec 21, 2018 •

edited

Loading

JoviDeCroock commented Dec 27, 2018

rosskevin commented Dec 27, 2018

JoviDeCroock commented Dec 27, 2018 •

edited

Loading

rosskevin commented Dec 27, 2018

rosskevin commented Dec 27, 2018

JoviDeCroock commented Jan 3, 2019

rosskevin commented Jan 3, 2019

rosskevin commented Jan 3, 2019

JoviDeCroock commented Jan 3, 2019

JoviDeCroock commented Jan 4, 2019 •

edited

Loading

rosskevin left a comment

JoviDeCroock commented Jan 4, 2019 •

edited

Loading

rosskevin commented Jan 4, 2019

JoviDeCroock commented Jan 5, 2019 •

edited

Loading

rosskevin commented Jan 7, 2019

JoviDeCroock commented Jan 7, 2019 •

edited

Loading

rosskevin commented Jan 7, 2019

chore: provide esm bundle #2677

chore: provide esm bundle #2677

Conversation

JoviDeCroock commented Dec 18, 2018 • edited Loading

JoviDeCroock commented Dec 20, 2018

benjamn commented Dec 21, 2018

JoviDeCroock commented Dec 21, 2018

rosskevin commented Dec 21, 2018

JoviDeCroock commented Dec 21, 2018 • edited Loading

benjamn commented Dec 21, 2018 • edited Loading

JoviDeCroock commented Dec 21, 2018 • edited Loading

JoviDeCroock commented Dec 27, 2018

rosskevin commented Dec 27, 2018

JoviDeCroock commented Dec 27, 2018 • edited Loading

rosskevin commented Dec 27, 2018

rosskevin commented Dec 27, 2018

JoviDeCroock commented Jan 3, 2019

rosskevin commented Jan 3, 2019

rosskevin commented Jan 3, 2019

JoviDeCroock commented Jan 3, 2019

JoviDeCroock commented Jan 4, 2019 • edited Loading

rosskevin left a comment

Choose a reason for hiding this comment

JoviDeCroock commented Jan 4, 2019 • edited Loading

rosskevin commented Jan 4, 2019

JoviDeCroock commented Jan 5, 2019 • edited Loading

rosskevin commented Jan 7, 2019

JoviDeCroock commented Jan 7, 2019 • edited Loading

rosskevin commented Jan 7, 2019

JoviDeCroock commented Dec 18, 2018 •

edited

Loading

JoviDeCroock commented Dec 21, 2018 •

edited

Loading

benjamn commented Dec 21, 2018 •

edited

Loading

JoviDeCroock commented Dec 21, 2018 •

edited

Loading

JoviDeCroock commented Dec 27, 2018 •

edited

Loading

JoviDeCroock commented Jan 4, 2019 •

edited

Loading

JoviDeCroock commented Jan 4, 2019 •

edited

Loading

JoviDeCroock commented Jan 5, 2019 •

edited

Loading

JoviDeCroock commented Jan 7, 2019 •

edited

Loading