Migrate webchannel-wrapper to gulp + google-closure-compiler #2891
Conversation
Binary Size ReportAffected SDKs
Test Logs
|
| resolve(__dirname, './externs/overrides.js'), | ||
| resolve(__dirname, './externs/module.js') | ||
| ], | ||
| language_out: 'ES5', |
There was a problem hiding this comment.
Could this be an option? I wonder if we would get size savings if we used ES2017 for our ES2017 build.
There was a problem hiding this comment.
Added an ES2017 build, size comparison:
47365 packages/webchannel-wrapper/dist/index.esm.js
41267 packages/webchannel-wrapper/dist/index.esm2017.js
47021 packages/webchannel-wrapper/dist/index.js
There was a problem hiding this comment.
Thanks! I think that is worth the effort. Do you know why the SDK size went up, despite this change?
There was a problem hiding this comment.
The problem was that previously all builds were actually ES2017, due to a bug in closure-builder. This was great for size but not great for ES5 compatibility. Now the builds that are supposed to be ES5 actually are (making them bigger) and a correctly-labeled ES2017 build has been added.
There was a problem hiding this comment.
I actually thought more about this - the size increase for the NPM builds are explained by these changes. The size increase for the CDN build is not though. The CDN build should have post processed everything to ES5 already. Hence, I think what is going on here is that the new build pipeline is no longer tree shaking code that Firestore isn’t using. Is that a possibility?
There was a problem hiding this comment.
So my explanation is incorrect for the ESM bundle which was already writing ES5, I think because of using an outdated library (see below). The bundle size increase is actually due to a change in closure-library from versions 20200112 to 20200204. I don't know exactly which change yet, but this led to a 5K bundle size spike on the webchannel-wrapper ES5 builds between those two closure-library versions.
Because webchannel-wrapper was using closure-builder, the ESM build was fixed on a pretty old version bundled with closure-builder. Once we upgraded it to use a newer version of google-closure-library (20200224, I copied the version auth was using) it saw a 5K size increase.
If we upgrade to the latest version of google-closure-library (20200406) the size increase is smaller (2.5K) but still there.
For now we might want to pin closure-library to 20200112 (before the change that caused this size increase) and try to figure out exactly where the increase is coming from.
There was a problem hiding this comment.
This LGTM to me (but I mostly just compared the previous to the new settings). Please wait for @Feiyang1 to review and consider adding ES2017 builds if this cuts down on bundle size in a significant way,
|
Pinning to closure-library and closure-compiler versions 20200112. Recent closure-library versions starting at 0204 increase the bundle size, although the most recent version (0406) has a smaller size increase. Latest closure-compiler doesn't cause problems but better to keep versions in sync? Unfortunately the esm2017 build using 0112 doesn't seem to be much of a size savings, but it does decrease by 3K with 0406 so seems like it's worth keeping in there for potential size savings when we're able to upgrade closure-library. |
|
Upgraded to latest closure deps. Using closure to compile to ES2017, then Typescript to compile to ES5. Final bundle sizes should be smaller now. "ES5" bundles do include Ideally auth should upgrade closure versions as well, but this currently causes auth tests to fail and would require more investigation. |
| typescriptPlugin({ | ||
| typescript, | ||
| // Uncomment for development only. Prevents caching between builds. | ||
| // clean: true, |
There was a problem hiding this comment.
Is it needed? I thought it is only necessary if we are doing custom source code transformation.
There was a problem hiding this comment.
Is Typescript needed or is clean needed? Lack of clean seems to cause problems when trying different Typescript config options during development and it seems good to remind myself/others to use it next time we try to tweak the config.
There was a problem hiding this comment.
clean. Should we always enable it then?
There was a problem hiding this comment.
I don't think it's needed if you're not changing the Typescript config, and caching saves time.
There was a problem hiding this comment.
When should I enable it, I don't think we are changing the config? Sorry that I'm nitpicking. If it is not really needed, I think we should just remove it.
There was a problem hiding this comment.
I want to leave a note for anyone in the future who has to tinker with the build pipeline so they don't forget to enable this when working, or they will get really frustrated when changes to tsconfig don't seem to do anything. But I don't want to leave it on for production and regular usage. I don't know if a comment is the best place to leave that note, but if it's outside the code I don't think anyone will remember to check.
Webchannel wrapper was previously using
closure-builderwhich bundles its specific version ofclosure-libraryand is not updated often. This led to a bug where it was configured to be compiling to an ES5 target but ignored that option (language_out) and compiled to ES2017. This could cause a bug in IE if the app is using untransformed code directly fromwebchannel-wrapper/dist, such as in rxfire tests run in IE.Moving to use gulp which directly uses
google-closure-compilerand correctly recognizes thelanguage_outattribute.