Repository navigation
Conversation
tyler-reitz
left a comment
There was a problem hiding this comment.
Requesting changes for one thing, which only shows up together with #839.
Tree-shaking works as described: on Vite 8.3.5 with firebase 12, an auth-only app goes from 240 KB to 82 KB gzipped with no Firestore, Database, Storage or Functions code. The 74 ESM and CJS exports match main, and #835's import-time registration survives "sideEffects": false.
Blocking: rxjs 6 apps. Externalizing rxjs and rxfire splits them: in an rxjs 6.6.7 app, reactfire's code loads a nested rxjs 7.8.2 while rxfire loads the app's 6.6.7, and teardown doesn't cross that boundary. On its own that's invisible, since shareReplay never tears down, but with #839 merged too, useUser and useFirestoreDocData each leak one listener per remount (1,2,3,4,5 after five cycles; 0 on rxjs 7). Making rxjs a ^7.1.0 peer dependency turns this into an install error. Whichever of this and #839 lands first should carry it, plus a !, and the body's "runtime behavior and npm install are unchanged" needs updating for these apps.
Non-blocking: target: es2021 also avoids the tslib chunk and keeps dist at ES2020; es2024 raises it to ES2022 (native class fields), one level above main.
Externalizing rxjs and rxfire adds no size win (the split ESM output alone gives it) and installs a second rxjs for rxjs 6 apps. Restore main's rxjs range and target es2021 so the output stays at ES2021.
auth, database and firestore imported ReactFireError and checkIdField from index.ts, which re-exports every product, so Turbopack kept all of them. Move the four values to src/utils.ts and re-export them from index.ts.
|
I'm withdrawing my rxjs peer-dependency and Externalizing rxjs saved nothing: bundling it is 0.1 to 0.5 kB smaller in every bundler, and it removes the nested rxjs 7 and the TS2345 error in rxjs 6 apps. Without
webpack, esbuild and Rollup drop unused products as well. Exports, attw, the release smoke test and the suite on React 18 and 19 match @armando-navarro could you review the rework? Jeff, I'll loop you in after. |
Superseded by the rework in 2d644f7, see comment.
…e_modules Node resolves a file's module format from the nearest package.json and stops that search at a folder named node_modules. The vendored rxjs, rxfire and tslib files therefore had no "type": "module" scope and were read as CommonJS on Node without default module syntax detection (before 20.19 and 22.7) and under Yarn Plug'n'Play, so importing reactfire there failed with "is a CommonJS module". Renaming the output folder keeps the files inside this package's scope. Bundle sizes, attw and publint are unchanged.
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks for the rework, Tyler, and for the original build change, Jeff. The size win is real: I rebuilt both tarballs, measured auth-only and Firestore-only consumer apps under Vite 7 and an auth-only app under Next 16 Turbopack, and got your table within a few kB, with only the matching @firebase packages left in each bundle.
One thing does regress against main, and it is the only reason I am not approving yet.
1. Blocking: the package no longer imports through Node's own ESM loader on Node before 20.19 / 22.7, nor under Yarn Plug'n'Play
Cause:
- The emitted modules import rxjs, rxfire and tslib from
dist/node_modules/..., and those folders ship without apackage.json. - Node's module-format lookup stops at any folder named
node_modules, so those files lose this package's"type": "module"and load as CommonJS. - Node 22.7.0 and 20.19.0 turned on module syntax detection by default, which is why it loads on current Node and in the release smoke test. Older Node, and Yarn PnP's own loader, do not detect.
What I ran with the PR tarball:
- npm install, Node 20.12.2,
import('reactfire'): fails with "Named export 'Subject' not found. The requested module './node_modules/rxjs/dist/esm5/internal/Subject.js' is a CommonJS module". - Yarn 4.12 PnP project (loose mode, since firebase's own undeclared peers already fail strict PnP) on Node 22.23.2: same error.
- The
maintarball loads in both.
Who is affected: any app that loads reactfire through Node itself rather than a bundler (server rendering, test runners), on Node 20 before 20.19 (end of life, but engines still says >=14) or under Yarn PnP, on current Node too.
Fix I verified, one line in vite.config.ts:
- Replace the ES output's
entryFileNamesstring with a function that maps thenode_modules/prefix tovendor/, so the vendored files inherit this package's module scope. - Node 20.12.2 and the PnP project both import the result.
- The auth-only Vite bundle stays at 106 kB with the same five
@firebasepackages. - I pushed it as branch
armando/838-vendor-folderon this repo, one commit (1fa546c) on top of2d644f7, in case cherry-picking is easier than retyping it.
A { "type": "module" } package.json placed in each of the three vendored folders also fixes Node 20.12.2 (not tested under PnP), but I would take the rename.
Everything below is optional and does not block the approve.
2. src/index.ts must define no runtime values, and nothing says so
- I rebuilt with a runtime export added to
index.tsand imported byauth.tsx. When rolldown cannot inline it (a function, a class, or a value that is re-exported),dist/auth.jsimports./index.jsby name again. - That is the exact path that made Turbopack keep every product before the barrel move.
- Nothing in CI would notice that: size-limit measures the whole package, and the tests import from
src. - A one-line comment at the top of
index.tssaying "types and re-exports only, define no runtime values here" would stop the next helper from undoing this silently.
3. The moved checkOptions keeps a cast the compiler does not need
npx tscpasses without theas ReactFireOptions['idField'] | ... | ReactFireOptions['suspense']cast.- The emitted declarations are byte-identical with or without it, because
checkOptions,checkIdFieldandcheckinitialDataare declaredanyeither way (initialDataisT | any). - Moved code, so whether to touch it in this PR is up to you.
4. build.lib.fileName: 'index' is now dead configuration
- Both outputs set
entryFileNamesexplicitly, so thefileNameentry influences nothing and a reader will expect it to. Worth removing while in the file.
If I have misread the Node resolution rules or there is a reason to keep the node_modules folder name, say so and I will look again.
| format: 'es', | ||
| preserveModules: true, | ||
| preserveModulesRoot: 'src', | ||
| entryFileNames: '[name].js', |
There was a problem hiding this comment.
This is the line behind item 1. With preserveModules, the bundled rxjs, rxfire and tslib land under dist/node_modules/, and Node stops its package.json scope lookup at that folder name, so the files lose this package's "type": "module" and fail to import on Node before 20.19 / 22.7 and under Yarn PnP. A function here that maps the node_modules/ prefix to vendor/ fixes it, with the auth-only Vite bundle unchanged at 106 kB.
| return checkOptions(options, 'idField'); | ||
| } | ||
|
|
||
| export { ReactFireError, checkOptions, checkinitialData, checkIdField } from './utils.js'; |
There was a problem hiding this comment.
Item 2. Moving these definitions out is what lets Turbopack drop unused products. A runtime function or class defined in this file and imported by any product module brings the regression back silently. Worth a one-line comment at the top of the file stating that this file holds types and re-exports only.
…leName Removing lib.fileName leaves dist/ byte-identical, since rollupOptions.output sets entryFileNames for every format.
|
Thanks, Armando. I took item 1 as your commit ( I reproduced item 1 before taking the fix. On Node 20.12.2, The packed file set is identical apart from the rename. Vite auth-only and all-hooks bundles are unchanged (104.8 and 289.3 kB). Firestore-only is 26 B smaller because rxjs's |
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks, Tyler. I re-ran the checks on 225fb26 and everything holds:
import('reactfire')loads on Node 20.12.2, on Node 22 with syntax detection off, and under Yarn PnP.- The auth-only bundles are unchanged: 106 kB under Vite 7 and 210 kB under Next 16 Turbopack, with only the five
@firebasepackages the app uses. - A full build leaves the reference docs unchanged.
Leaving item 3 as is works for me.
Summary
dist/index.jswas a single file that imported every Firebase product, so an app using onlyuseUserstill bundled Firestore, RTDB, Storage, Functions, Remote Config and Performance. This PR:preserveModules: true,preserveModulesRoot: 'src',entryFileNames: '[name].js') alongsidedist/index.umd.cjs. With the"sideEffects": falsealready onmain(ci: add built-artifact release gate (#765) #766), bundlers can now skip unused product modules and theirfirebase/*imports.ReactFireError,checkIdField,checkOptionsandcheckinitialDatafromsrc/index.tstosrc/utils.ts, still exported fromindex.ts.auth,databaseandfirestoreimported them back from the barrel, and Next 16's default bundler (Turbopack) then kept every product.tsconfigtargettoes2021(fromes2015), so the split output stays at ES2021, the levelmainalready ships.rxjs and rxfire stay bundled, as on
main; underpreserveModulesthey are emitted todist/vendor/. Notdist/node_modules/: Node's package scope lookup stops at that folder name, so the files would load as CommonJS wherever Node does not detect module syntax (before 20.19 / 22.7, and Yarn PnP). The rxjs range is unchanged (^6.6.3 || ^7.0.1).Bundle size
Consumer apps installed from a packed tarball, firebase 13.1.0, React 19.3.1. gzip -9 of all emitted JS, kB.
mainimport * as RF)--webpack, auth onlywebpack 5, esbuild and Rollup also drop unused products. Without the change in (2), Turbopack keeps all six products (384.8 kB).
Compatibility
importandrequire;main,exportsandtypingsunchanged.useObservable:tscpasses, no nested rxjs.main.Risks
dist/vendor/ships in the package (rxjs, rxfire, tslib; 87 files). Node ESM imports it on 20.12.2 and 22.15 (thedist/node_modules/layout failed on 20.12.2). Yarn PnP: Armando's run imports it; not run by me.sideEffects, e.g.firebase/databaseregistering its component in an auth-only app. An app that callsgetDatabaseimportsfirebase/databaseitself, so this is expected to be harmless (inferred, not tested beyond the smoke test).Closes #489
Refs #488 (lazy-loading behind
React.lazywas not measured)Refs #480