firebase-js-sdk icon indicating copy to clipboard operation
firebase-js-sdk copied to clipboard

firebase/auth and firebase/database treeshaking please

Open fserb opened this issue 3 years ago • 7 comments

Current firebase: 9.9.1

Some of firebase's modules are tree-shaking friendly, but some still aren't. In particular, firebase/database and firebase/auth.

For example, I have a module that doesn't need to do actual authentication, but just get persistence storage, and generate tokens. But even just importing getAuth (import {getAuth} from "firebase/auth";) still brings +300kb out of the file's 400kb to the rollup bundle (80kb after terser, etc, but still). Things that get included: all the *AuthProvider(); all *Persistence(); Subscription, Redirects, etc.

I know that the new philosophy behind the SDK is to support function-based import, and I think some other modules saw a big difference. But this still hurts quite a bit.

A similar problem with firebase/database. If you only want to get/set values from the realtime database, you must import almost the whole file for something that can be actually replaced by a fetch with token to the REST API.

Right now, doing:

import {initializeApp} from "firebase/app";
import {getAuth} from "firebase/auth";
import {getDatabase} from "firebase/database";

const app = initializeApp(firebaseConfig);
const auth = getAuth(app);
const db = getDatabase(auth);

and bundling with rollup generates a 738kb pre-terser (193kb after) and those two modules are the biggest offender.

Maybe there's a magical rollupConfig.treeshake option that I'm missing, but it would be nice to have a solutions for those.

fserb avatar Aug 04 '22 01:08 fserb

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

google-oss-bot avatar Aug 04 '22 01:08 google-oss-bot

For Auth, getAuth() pulls in many dependencies that it sounds like you don't need (it's meant to be all-encompassing, to cover most use cases). To take advantage of full tree-shaking, you'll want to use initializeAuth() instead. For example, read through https://firebase.google.com/docs/auth/web/custom-dependencies#tailoring_your_dependencies which shows how to pare down the Auth bundle for a similar use case to yours.

sam-gc avatar Aug 04 '22 17:08 sam-gc

Sam is correct that using initializeAuth() over getAuth() reduces bringing in some unneeded code. I also looked specifically into one symbol you mentioned being brought in that shouldn't be, the *AuthProvider or GoogleAuthProvider specifically. It seems like what's going on there is that in the source code, a couple of static readonly variables are being set on that class: https://github.com/firebase/firebase-js-sdk/blob/301bfcdbe9f925e4b021d22dd6106f24518fa001/packages/auth/src/core/providers/google.ts#L71

And then somehow during the Rollup/Typescript build, when using ESM2017 settings, the final dist file ends up compiling that to this, at the top level of code:

/** Always set to {@link SignInMethod}.GOOGLE. */
GoogleAuthProvider.GOOGLE_SIGN_IN_METHOD = "google.com" /* GOOGLE */;
/** Always set to {@link ProviderId}.GOOGLE. */
GoogleAuthProvider.PROVIDER_ID = "google.com" /* GOOGLE */;

So when the developer is building the app with Rollup or Webpack, and it sees this, that means the class is "used" and won't be tree-shaken out. I tried removing these 2 lines from node_modules/@firebase/auth in my test app and it seems like it was then able to tree-shake out GoogleAuthProvider. (Edit: I added them back in as static properties inside the class and it brought the class back in again.)

I'm not sure if there's something in Rollup or TS configs to avoid doing this, but I suspect this specific thing probably isn't the only thing contributing to suboptimal tree-shaking in our bundled code, and we'd probably have to take some time and do a full audit to catch enough to make a big difference. Since this is an important feature, we can put it on our roadmap but it won't be a quick fix.

As for RTDB, I'll also see if I can find anything that stands out, but I do know that we did a very fast modularization of RTDB and didn't have time to go through and really focus on breaking up the various components for maximal tree-shaking, so I'm guessing this is a reflection of that. If that's the case, we can put it on the list but it depends on the resources and priority of the RTDB team.

However, I'll look through it and if there's any quick wins (a Typescript or Rollup config fix that will address some obvious issues), I will definitely try to implement them sooner rather than later.

hsubox76 avatar Aug 04 '22 18:08 hsubox76

@sam-gc, thanks. initializeAuth() does improve the auth import quite a bit (I think it drops more than 100kb of code). The part of firebase/auth that I use (User + browserLocalPersistence + getIdToken) end up around 120kb after removing getAuth(). It's not tiny, but much more reasonable. :)

@hsubox76 regarding the static property, does it get converted back to an assignment in JS? It could be that the TS config is set to a JS target version that doesn't have support for it yet. I think static properties are either a 2021 or 2022 thing.

for the RTDB, what I did for now is replace my code to just use the REST API directly, as I just need to get/set some values. But yeah, it would be nice to use the SDK instead.

Anyways, thank you both so much for the attention and support. :)

fserb avatar Aug 05 '22 02:08 fserb

sorry to hijack my own issue, but what is the correct way to wait for initializeAuth() to finish?

the Promise floats: https://github.com/firebase/firebase-js-sdk/blob/cdada6c68f9740d13dd6674bcb658e28e68253b6/packages/auth/src/core/auth/initialize.ts#L83-L86

which means I can't do:

const auth = initializeAuth(app, {persistence: browserLocalPersistence});
console.log(auth.currentUser);

Is this a bug? Should initializeAuth return a Promise<Auth> instead? Or is the expectation that I still have to call setPersistence()? It's weird because if I just wait currentUser eventually gets populated.

fserb avatar Aug 05 '22 14:08 fserb

I ended up doing this:

await new Promise(acc => {
  const unsub = onAuthStateChanged(auth, () => { unsub(); acc(); });
});

right after initializeAuth(), but it seems a bit hack-y.

fserb avatar Aug 05 '22 14:08 fserb

The promise does float, but it's no different than getAuth, which immediately returns initializeAuth under the hood: https://github.com/firebase/firebase-js-sdk/blob/a80e29cbb90662866ee0ac0b33882ba8b9c62efa/packages/auth/src/platform_browser/index.ts#L37-L52

Auth will maintain internal state properly regardless of initialization (meaning: you can immediately call sign in functions, etc, and the library will properly handle this). The only thing that's not available immediately after initialization is currentUser. We recommend using the onAuthStateChanged() listeners anyway instead of currentUser as the value returned from the listener will always be correct.

sam-gc avatar Aug 05 '22 16:08 sam-gc

I have exact same problem, anyone get the solution yet? I use the firebase 9.15.0 version, see the PR here: https://github.com/shinaBR2/shinabr2-world/pull/56/files

In my code, here is how I import the modules

import { initializeApp } from "firebase/app";
import { getFirestore } from "firebase/firestore";

I can not believe how can only two functions can take 475.47 KB after tree-shaked.

shinaBR2 avatar Jan 08 '23 03:01 shinaBR2

Hm, I'm not familiar with turbo but I built a webpack test app with those imports and compiled it in webpack's "production" mode:

import {initializeApp} from "firebase/app";
import {getFirestore} from 'firebase/firestore';

//your firebase config
const firebaseConfig = { ... };

const app = initializeApp(firebaseConfig);
const firestore = getFirestore(app);
console.log(firestore.app);

and the bundle size was 95K, which is not amazing but is a lot different from 475K. Is there a production mode setting or minification/terser settings?

hsubox76 avatar Jan 09 '23 17:01 hsubox76

I will get back later since I think I miss something but not sure for now.

shinaBR2 avatar Jan 10 '23 02:01 shinaBR2

I've been following this issue as we were also seeing bundle size larger than hoped when we first looked at upgrading to modular last year.

In case it helps others, we've just noticed that part of the issue is that 88.2kB (30%) of our bundled output is repeated license comments imported from the original individual source files. I've posted a full description of that as a separate issue at #7500, which includes a workaround to configure vite / esbuild to remove these.

acoulton avatar Jul 28 '23 14:07 acoulton

Using a single LICENSE file would certainly be preferable both for license management purposes and for bundle size.

Nantris avatar Jul 28 '23 22:07 Nantris