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.

Sign up for GitHub

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

Jump to bottom

Add early experiments for tree-shakable ES5 output. #3407

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add early experiments for tree-shakable ES5 output. #3407

wants to merge 2 commits into from

Conversation

yuchenshi
Copy link
Member

DO NOT MERGE! This is early experiment results.

See my test script and test results at https://gist.github.com/yuchenshi/78ded5074e1e281306a17782d010c0e5

BEFORE

->shake$ bash test.bash 
=== Rolling up index.es2017.js ===

npx: installed 2 in 0.991s

index.es2017.js → stdout...
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
@firebase/app-exp (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
@firebase/app (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
@firebase/logger (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
util (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
crypto (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
@grpc/proto-loader (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
@grpc/grpc-js (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
path (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
protobufjs (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
@grpc/grpc-js/package.json (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
@firebase/component (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)
@firebase/util (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.esm2017.js)

=== Searching for references of unused CollectionReference in ES2017 bundle ===

No references (good)!
File sizes (FYI):
 64K    bundle.es2017.js

=== Rolling up index.es5.js ===

npx: installed 2 in 1.009s

index.es5.js → stdout...
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
@firebase/app-exp (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
tslib (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
@firebase/app (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
@firebase/logger (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
util (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
crypto (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
@grpc/proto-loader (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
@grpc/grpc-js (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
path (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
protobufjs (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
@grpc/grpc-js/package.json (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
@firebase/component (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)
@firebase/util (imported by ../../firebase-js-sdk/packages/firestore/dist/lite/index.node.es5.js)

=== Searching for references of unused CollectionReference in ES5 bundle ===

        return new CollectionReference(ResourcePath.fromString(pathString), this, 
            return new CollectionReference(this._key.path.popLast(), this.firestore, this._converter);
        return new CollectionReference(this._key.path.child(path), this.firestore, 
var CollectionReference = /** @class */ (function (_super) {
    __extends(CollectionReference, _super);
    function CollectionReference(_path, firestore, _converter) {
    Object.defineProperty(CollectionReference.prototype, "id", {
    Object.defineProperty(CollectionReference.prototype, "parent", {
    Object.defineProperty(CollectionReference.prototype, "path", {
    CollectionReference.prototype.doc = function (pathString) {
        validateBetweenNumberOfArgs('CollectionReference.doc', arguments, 0, 1);
        validateArgType('CollectionReference.doc', 'non-empty string', 1, pathString);
    CollectionReference.prototype.add = function (value) {
        validateExactNumberOfArgs('CollectionReference.add', arguments, 1);
        validateArgType('CollectionReference.add', 'object', 1, convertedValue);
    CollectionReference.prototype.withConverter = function (converter) {
        return new CollectionReference(this._path, this.firestore, converter);
    return CollectionReference;
var CollectionReference$1 = /** @class */ (function (_super) {
    __extends(CollectionReference, _super);
    function CollectionReference(firestore, _path, _converter) {
    Object.defineProperty(CollectionReference.prototype, "id", {
    Object.defineProperty(CollectionReference.prototype, "path", {
    CollectionReference.prototype.withConverter = function (converter) {
        return new CollectionReference(this.firestore, this._path, converter);
    return CollectionReference;
File sizes (FYI):
1.0M    bundle.es5.js

After:

=== Searching for references of unused CollectionReference in ES5 bundle ===

No references (good)!
File sizes (FYI):
 92K    bundle.es5.js

Note the huge 1M -> 92K size reduction due to tree shaking.

@changeset-bot
Copy link

changeset-bot bot commented Jul 14, 2020 •
edited
Loading

💥 No Changeset

Latest commit: 7827f89

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 14, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/firestore/lite

    Type Base (c2b737b) Head (1172bcf) Diff
    main 124 kB 152 kB +27.7 kB (+22.3%)

Test Logs

@schmidt-sebastian
Copy link
Contributor

From a technical standup, this LGTM. I still don't think that this fixes a problem worth fixing though, and adding 22.3% to the SDK build that most people will consume seems like a non-starter.

@yuchenshi
Copy link
Member Author

From a technical standup, this LGTM. I still don't think that this fixes a problem worth fixing though, and adding 22.3% to the SDK build that most people will consume seems like a non-starter.

I think ultimately it's up to you and Fei and others to decide, but I'd love to list all of our options here. I think we can do either ES2017-only, or ES5-only, or both. I'd say if we go ES5-only (or ES5 by default), this is almost a must-have. If we do both, having tree-shakable ES5 would be still quite nice, but we can also just say "use ES2017 build if you want even better tree-shaking".

@yuchenshi yuchenshi force-pushed the ys/experimental-es5-firestore-lite branch from eef18ba to 7827f89 Compare July 14, 2020 23:21
@schmidt-sebastian
Copy link
Contributor

From a technical standup, this LGTM. I still don't think that this fixes a problem worth fixing though, and adding 22.3% to the SDK build that most people will consume seems like a non-starter.

I think ultimately it's up to you and Fei and others to decide, but I'd love to list all of our options here. I think we can do either ES2017-only, or ES5-only, or both. I'd say if we go ES5-only (or ES5 by default), this is almost a must-have. If we do both, having tree-shakable ES5 would be still quite nice, but we can also just say "use ES2017 build if you want even better tree-shaking".

Unfortunately, ES5 by default doesn't address my size concern since almost all users will just pull in the default SDK version (evidence being that our ES2017 build was broken for years).
I can live with an optional ES5 build, but I suspect that the users that know how to use a custom import probably also know how to configure Babel correctly. The additional build time is also a tad bit annoying (but not a blocker).

@Feiyang1
Copy link
Member

Feiyang1 commented Jul 15, 2020 •
edited
Loading

Thanks for exploring this option! I think the solution is quite neat (very short and clean to apply).

Regarding the build targets for next, I'm leaning toward using es2015+ for files that are in esm format, that includes browser and module fields, and using es5 for the main field which will be in umd (Since umd is not tree shakable by its nature, we won't need to apply this technique). This is what angular packages do today, see https://unpkg.com/browse/@angular/[email protected]/package.json
@jamesdaniels @davideast Do you have an opinion on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-gc sam-gc sam-gc left review comments

@Feiyang1 Feiyang1 Awaiting requested review from Feiyang1

@schmidt-sebastian schmidt-sebastian Awaiting requested review from schmidt-sebastian

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yuchenshi @google-oss-bot @schmidt-sebastian @Feiyang1 @sam-gc