Generate smaller bundles (take 2)

[jsayol]

This reapplies the uglify optimizations from #94, which were lost after the build process was switched to rollup a while ago.

Here's the size diff compared with the current master:

File	Before	After	% diff
firebase-app.js	34.03 KiB	32.63 KiB	-4.12%
firebase-database.js	178.22 KiB	158.28 KiB	-11.19%
firebase-firestore.js	354.32 KiB	350.68 KiB	-1.03%
firebase-functions.js	7.30 KiB	7.26 KiB	-0.51%
firebase-messaging.js	35.01 KiB	34.11 KiB	-2.55%
firebase-storage.js	35.57 KiB	32.05 KiB	-9.90%
firebase.js	779.88 KiB	751.72 KiB	-3.61%

This change doesn't affect firebase-auth.js, since applying these uglify options actually generated a slightly larger bundle for auth. It does apply to the global firebase.js, though, which includes auth.

All tests pass.

[jsayol]

It seems like something went wrong on the Travis build, I'll look into it later.

Just a glitch with Travis. The tests passed correctly the second time.

[Feiyang1]

Does it mean it could break things in some case?

[jsayol]

The compress.unsafe option applies these 4 transformations:

• new Array(1, 2, 3) or Array(1, 2, 3) → [ 1, 2, 3 ]
• new Object() → {}
• String(exp) or exp.toString() → "" + exp
• new Object/RegExp/Function/Error/Array (...) → the new is discarded

These should actually be safe in most cases. That being said, I just did a build without that option and it seems like it only provides a very marginal reduction in size, about an extra 0.2% on average.

I don't think it's worth it, so I'll remove it.

Edit: I also removed the compress.warnings option, since it turns out the default is already false.

[Feiyang1]

It looks great, thank you!

[Feiyang1]

LGTM

[Feiyang1]

Have you done some smoke tests with this change? Unfortunately our CI build doesn't test the minified bundle. It's probably a good idea to do some testing with a test app before merging. I'm going to do it myself as well.

[jsayol]

I thought the tests were also run against the generated bundles. I might be wrong, but wasn't that the case about a year ago?

I'll do some smoke testing, but I think it might be a good idea to add some tests of the minified files at some point.

[mikelehen]

I think they are not. :-( Unit tests, etc. of course won't work against the minified build. A while ago, I split out the Firestore tests that could be run against a minified build. firebase-js-sdk/integration/firestore used to do that, but it was removed from CI and then I think got broken by some build refactorings at some point, so it probably doesn't work anymore.

We should probably re-invest in some tests against the minified builds. But until then, changes like this come with some risk we'll need to mitigate through manual testing or similar.

[jsayol]

Ok, database works fine but there's definitely something wrong with the Firestore build. Getting lots of INTERNAL ASSERTION FAILED, I'm looking into it.

[jsayol]

Fixed the issue, sorry about this.

I've run the samples from https://github.com/firebase/quickstart-js against my local build and everything is working fine now. I've tested Auth (anonymous and google sign-in), RTDB, Firestore, FCM, and Storage.

[Feiyang1]

@jsayol Interesting. Why does mangling double underscore broke the component? Now I'm a little concerned about the property mangling because it's not safe (the uglify page also mentioned it). Are there more patterns we need to exclude? I wonder if it is why it was removed when refactoring.
@mikelehen what do you think? what is our convention of using _ vs __ and prefix _ vs postfix _.

[jsayol]

Unless we can run all the test cases on the minified bundle, I'm not very comfortable making this change.

Yeah, I agree. This isn't necessary anyway so it might be better to wait until there's at least some tests on the minified files.

Why does mangling double underscore broke the component?

This one's a bit tricky, and I'm not really happy with the fix. For some reason uglify is ignoring the keep_quoted: true option (maybe it doesn't play well with other conditions like regex). The goog.labs.net.webchannel package from the google-closure-library uses channelMsg['__data__'] to get the message data. Uglify was mangling __data__ to something else, causing the message data to get lost, and that made Firestore fail.

That's definitely not an ideal situation, since similar errors might happen again in the future with any other quoted properties that begin or end with an underscore. I'll keep looking into it and see if I can figure out what's going on with keep_quoted.

[mikelehen]

Here's where it was removed: https://github.com/firebase/firebase-js-sdk/pull/653/files#diff-f8545829bd5e24a8a540f499f5878fa7L57 I'm guessing it was just an accident. That PR was massive.

It looks like the old regex was /^_[^_][^lat]|[^_]_$/. So without [^lat], this PR will likely break auth (see #238 and #230). (sidenote: I think [^lat] probably should have been (?!lat) instead.)

So yeah, I agree this isn't a very safe change. If we put it back to exactly what it was before, it may be fine, but there's a chance we have new weirdly-named properties we don't know about and won't find without exhaustive testing.

AFAIK we also don't use _foo or foo_ conventions very much in our code, so this regex won't help reduce code size all that much (and incidentally, to measure meaningfully, we should look at the size after gzip).

That said, there is one nice benefit of the mangling we used to do. It helps us hide non-public properties on our APIs. So Firestore uses the _foo convention in all of our public API classes (e.g.

firebase-js-sdk/packages/firestore/src/api/database.ts

Line 240 in 97cc3f8

private readonly _config: FirestoreConfig;

), but we don't bother in the rest of our code. Without mangling these properties, people may notice them while debugging and then take a dependency on them in their app. If they do, then we may someday break their app when making seemingly-innocuous changes to our implementation details. With name mangling, the names would end up unrecognizable and automatically change from release to release, so devs couldn't depend on them.

@jsayol Out of curiosity, did you have a motivation for this change beyond the size reduction? Also did you by chance look at the post-gzip size difference to see if that erodes the savings much?

Overall I'd say that given the state of things (moderate risk of regression, low-ish benefit since we don't use the conventions much), this change may not be worth it. 😕

[jsayol]

Out of curiosity, did you have a motivation for this change beyond the size reduction?

Not really. I initially did this last year because the database bundle had exploded in size after porting it to TypeScript and moving away from the closure compiler. A few days ago I was debugging something elsewhere on the SDK and noticed a _foo-style property that shouldn't have been there, so I realized the uglify changes had been lost and I figured I might as well reapply them.

So without [^lat], this PR will likely break auth (see #238 and #230).

Yup, definitely will. And I think this case wouldn't be covered even if keep_quoted was being applied correctly, so it would still need an exception.

So yeah, I agree that this is not a safe approach overall. I'm going to close this PR unless anyone says otherwise. Thanks for the patience and for looking into all this :)

P.S.: Just a side note, have you given any thought to using closure compiler again but merely as a minifier replacing uglify? This rollup plugin looks interesting. I actually played with it earlier as a possible alternative here, but I have zero experience configuring closure compiler so I didn't get very far. Might be worth it, though, unless reducing the bundle size is not really a priority.

Edit: Forgot about this:

Also did you by chance look at the post-gzip size difference to see if that erodes the savings much?

-1.97% firebase-app.js.gz
-4.50% firebase-database.js.gz
-0.35% firebase-firestore.js.gz
-0.54% firebase-functions.js.gz
-1.65% firebase-messaging.js.gz
-3.91% firebase-storage.js.gz
-1.52% firebase.js.gz

[Feiyang1]

One of our top priorities is to modularize this SDK, so you can pull in the code you really use instead of the entire package, e.g. just signInWithEmailPassword instead of the entire auth SDK. It would require developers to use the SDK as modules and to utilize bundler's tree shaking capability.

Clearly it won't help with the size of the minified bundles since they always include the entire component. We see the minified bundles as a convenient way to do experiments and quick prototypes, but discourage its usage in production in favor of ES6 modules. If you chose to use the minified bundle, you are already missing out the opportunity for size reduction, so I don't think we will focus too much on the minified bundle size.

[jsayol]

One of our top priorities is to modularize this SDK, so you can pull in the code you really use instead of the entire package

That makes perfect sense. From what you say I assume that's something the team is already working on internally? If not, I'd be happy to look into it.

I was thinking about this a couple days ago and it would require a slightly different API. The best approach would be if the public API methods were pure functions whenever possible, in particular doing away with any shared state between them.

So as an example, using database and auth could look like this:

import { initializeApp } from 'firebase/app';
import {
  auth,
  signInWithEmailAndPassword,
  onAuthStateChanged
} from 'firebase/auth';
import {
  database,
  reference,
  child,
  query,
  orderByValue,
  onValue
} from 'firebase/database';
import { config } from './config';

const app = initializeApp(config);
const authClient = auth(app);
const databaseClient = database(app, { auth: authClient });

const email = '[email protected]';
const password = 'bob123';
signInWithEmailAndPassword(authClient, email, password);

onAuthStateChanged(authClient).then(user => {
  if (user) {
    let dbRef = reference(databaseClient, 'userPosts');
    dbRef = child(dbRef, user.uid);

    let dbQuery = query(dbRef);
    dbQuery = orderByValue(dbQuery);

    onValue(dbQuery, snap => {
      console.log(snap.val());
    });
  }
});

This would allow a bundler to only include the methods that have been imported, along with what those methods use themselves.

The main issue I see, at least with the database package, is that all the internals are very tightly coupled so even if something like this is done, most of the package would still need to be pulled in.

Then there's also the fact that the whole auth package would need to be refactored as TypeScript, or as ES modules at the very least. But a tool like gents could probably help with that.

[jsayol]

Just dumping some more thoughts here.

In certain cases it could make sense to offer a method to cascade a series of operations on an object, since repeatedly passing that object to those functions might not be very readable. I'm thinking of something akin to rxjs' .pipe().

So for example, performing a query in Firestore could look like this:

const app = initializeApp(config);
const firestoreClient = firestore(app);

const query = collection(firestoreClient, 'shopOrders').pipe(
  where('customer', '==', customerId),
  where('purchased', '<', someDate),
  orderBy('orderId'),
  limitTo(100)
);

getSnapshot(query).then( /* ... */);

P.S.: "pipe" is actually a bad name for it, since that could be confusing for those using firebase with rxjs. Just used it to illustrate my point.

[Feiyang1]

These are all great points! Yes, we are actively looking at it internally. You are right to the point about making methods pure functions. At the same time we want to strike a balance between modularization (size reduction potential) and ease of use for developers.

And yes, some components will go through major rewrite. Please stay tuned! :)