Pre-process all ES5 builds #2840

schmidt-sebastian · 2020-03-31T04:55:46Z

@Feiyang1 I have to test these builds but this might enable tree-shaking in ES3/5 builds. I got this to work by using two different rollup phases.

google-oss-bot · 2020-03-31T05:03:48Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`d9b599f`)	Head (`982bbc1`)	Diff
@firebase/firestore/memory	browser	223866.00	212770.00	-11096.00 (-4.96%)
	module	221046.00	211136.00	-9910.00 (-4.48%)
	main	396522.00	376777.00	-19745.00 (-4.98%)
@firebase/firestore	browser	270246.00	269110.00	-1136.00 (-0.42%)
	module	267025.00	267081.00	+56.00 (+0.02%)
	main	487081.00	487448.00	+367.00 (+0.08%)
firebase	firebase.js	846074.00	846328.00	+254.00 (+0.03%)
	firebase-firestore.memory.js	266153.00	256172.00	-9981.00 (-3.75%)
	firebase-firestore.js	311197.00	311240.00	+43.00 (+0.01%)

Metric Unit: byte

Test Logs

Feiyang1 · 2020-03-31T16:57:22Z

LGTM. I'm glad (surprised?) to see that it reduces the memory only build significantly. It's also interesting to see that the size of firebase-firestore.js and firebase.js increased a tiny bit, probably because the mangling is done indirectly now for cjs and es5 builds? In that case, I don't think it is a problem.

schmidt-sebastian · 2020-03-31T17:23:38Z

Validation checklist:

[x] Browser builds are mangled
[x] Node builds are not mangled
[x] Non-ES2017 builds are ES3.
[x] ES2017 builds are ES2017
[X] firebase.js works
[X] firebase-firestore.js works
[X] firebase-firestore.memory.js works
[x] Node build works in Node 6
[x] Node memory-only build works in Node 6
[x] CJS build works in browser
[x] CJS memory-build works in browser
[x] ESM build works in browser
[x] ESM memory-build works in browser
[x] All builds have sourcemaps

schmidt-sebastian · 2020-03-31T20:59:25Z

@Feiyang1 This should now be ready for an actual review. I manually tested all builds.

Feiyang1 · 2020-03-31T22:18:55Z

packages/firestore/rollup.config.es2017.js

@@ -45,6 +48,10 @@ import {
 // The in-memory builds are roughly 130 KB smaller, but throw an exception
 // for calls to `enablePersistence()` or `clearPersistence()`.
 //
+// We use two different rollup pipelines to take advantage of tree shaking in


Probably mention that it's because rollup is not able to treeshake Typescript classes compiled to ES5.

Feiyang1 · 2020-03-31T22:21:55Z

packages/firestore/rollup.config.es.js

+    },
+    include: ['dist/*.js']
+  }),
+  terser({


Do we need terser again here since we already do it with the same options (comments: all, beautify: true) when producing esm2017 builds?

I believe the size went up without Terser here, but I will do another round to verify.

Update: The size without Terser is about 4% larger (which I don't quite understand either, since we Terser again before computing the size). I added it back.

IIUC, we are not doing property mangling here, and we don't care about the size saving by mangling variable names. Developers will do them when building their apps.
Removing terser here can save some build time.

Ok, removed.

Feiyang1 · 2020-03-31T22:43:12Z

packages/firestore/rollup.config.es.js

+import memoryPkg from './memory/package.json';
+
+// This file defines the second rollup pipeline and transpiles the ES2017 SDK
+// into ES3 code. By splitting the build process into two independent build


I think it should be ES5 code.

I forgot to look in base configuration and assumed we used the default, which is ES3. Fixed.

Feiyang1 · 2020-03-31T22:49:05Z

packages/firestore/rollup.config.es.js

+    typescript,
+    compilerOptions: {
+      allowJs: true,
+      importHelpers: true


It can be removed because It's already set in the root tsconfig.

This didn't work for me when it was not included yesterday, but I will try again. Fingers crossed :)

Update: No size impact so I removed it.

Feiyang1 · 2020-03-31T22:53:34Z

packages/firestore/package.json

@@ -26,6 +26,7 @@
    "prepare": "yarn build"
  },
  "main": "dist/index.node.cjs.js",
+  "mainES2017": "dist/index.node.esm2017.js",


main-esm2017 to be consistent with the existing convention? We have a lite-esm2017 target in @firebase/app which is used to produce firebase-performance-standalone.es2017.js

I was looking for a precedent. I am glad there is one :)

Feiyang1 · 2020-03-31T22:54:05Z

packages/firestore/memory/package.json

@@ -2,6 +2,7 @@
  "name": "@firebase/firestore/memory",
  "description": "A memory-only build of the Cloud Firestore JS SDK.",
  "main": "../dist/index.memory.node.cjs.js",
+  "mainES2017": "../dist/index.memory.node.esm2017.js",


main-esm2017?

Feiyang1 · 2020-04-01T17:23:52Z

packages/firestore/rollup.config.es2017.js

@@ -45,6 +48,11 @@ import {
 // The in-memory builds are roughly 130 KB smaller, but throw an exception
 // for calls to `enablePersistence()` or `clearPersistence()`.
 //
+// We use two different rollup pipelines to take advantage of tree shaking,
+// which Rollup only supports in pure ES2017 builds. The build pipeline in this


This is not accurate. rollup can tree shake ES5 builds as long as they are in the ESM format. The limitation is specific to the Typescript classes compiled down to ES5.

Also do you mean post-tree shaken ES2017 builds? IIUC, treeshaking is done in this phase, the second phase just changes the module format.

Updated the comment. I did mean "pre tree-shaken", as in "these builds are already tree shaken", which is the same as "post tree-shaken". Hence, I dropped the term.

schmidt-sebastian · 2020-04-01T17:48:11Z

I also updated the PR to remove the error handling for memory externs, which was meant to ensure that our ES5 builds don't accidentally pull in sources they don't need. If this happens now, Rollup will just tree shake those files.

Pre-process all ES3 builds

0027ac1

schmidt-sebastian force-pushed the mrschmidt/splitrollup branch from d01322f to 0027ac1 Compare March 31, 2020 05:47

Fix build

2ac59f1

schmidt-sebastian added 3 commits March 31, 2020 10:42

Fix up

3e176e4

Mangling for Browser builds

9fd0e0d

Better docs

3545adf

schmidt-sebastian requested a review from Feiyang1 March 31, 2020 20:58

schmidt-sebastian assigned Feiyang1 Mar 31, 2020

schmidt-sebastian added the api: firestore label Mar 31, 2020

Feiyang1 reviewed Mar 31, 2020

View reviewed changes

Feiyang1 assigned schmidt-sebastian and unassigned Feiyang1 Mar 31, 2020

schmidt-sebastian changed the title ~~Pre-process all ES3 builds~~ Pre-process all ES5 builds Apr 1, 2020

schmidt-sebastian force-pushed the mrschmidt/splitrollup branch from 3da5216 to 2b6a594 Compare April 1, 2020 05:25

Review

3a26d5e

schmidt-sebastian force-pushed the mrschmidt/splitrollup branch from 2b6a594 to 3a26d5e Compare April 1, 2020 05:33

Add Terser back

b5653a7

schmidt-sebastian removed their assignment Apr 1, 2020

Feiyang1 reviewed Apr 1, 2020

View reviewed changes

Review

8926cd1

Feiyang1 approved these changes Apr 1, 2020

View reviewed changes

Merge branch 'master' into mrschmidt/splitrollup

59744b1

schmidt-sebastian merged commit e062933 into master Apr 1, 2020

schmidt-sebastian deleted the mrschmidt/splitrollup branch April 6, 2020 19:26

firebase locked and limited conversation to collaborators May 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre-process all ES5 builds #2840

Pre-process all ES5 builds #2840

schmidt-sebastian commented Mar 31, 2020 •

edited

Loading

google-oss-bot commented Mar 31, 2020 •

edited

Loading

Feiyang1 commented Mar 31, 2020

schmidt-sebastian commented Mar 31, 2020 •

edited

Loading

schmidt-sebastian commented Mar 31, 2020

Feiyang1 Mar 31, 2020

schmidt-sebastian Apr 1, 2020

Feiyang1 Mar 31, 2020

schmidt-sebastian Apr 1, 2020

Feiyang1 Apr 1, 2020 •

edited

Loading

schmidt-sebastian Apr 1, 2020

Feiyang1 Mar 31, 2020

schmidt-sebastian Apr 1, 2020

Feiyang1 Mar 31, 2020

schmidt-sebastian Apr 1, 2020

Feiyang1 Mar 31, 2020

schmidt-sebastian Apr 1, 2020

Feiyang1 Mar 31, 2020

schmidt-sebastian Apr 1, 2020

Feiyang1 Apr 1, 2020

schmidt-sebastian Apr 1, 2020

schmidt-sebastian commented Apr 1, 2020

Pre-process all ES5 builds #2840

Pre-process all ES5 builds #2840

Conversation

schmidt-sebastian commented Mar 31, 2020 • edited Loading

google-oss-bot commented Mar 31, 2020 • edited Loading

Binary Size Report

Affected SDKs

Test Logs

Feiyang1 commented Mar 31, 2020

schmidt-sebastian commented Mar 31, 2020 • edited Loading

schmidt-sebastian commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feiyang1 Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Apr 1, 2020

schmidt-sebastian commented Mar 31, 2020 •

edited

Loading

google-oss-bot commented Mar 31, 2020 •

edited

Loading

schmidt-sebastian commented Mar 31, 2020 •

edited

Loading

Feiyang1 Apr 1, 2020 •

edited

Loading