Memory-only Firestore build #2608

schmidt-sebastian · 2020-02-08T06:33:55Z

This PR exposes a Memory-only version of Firestore that is 50 KB smaller than our current build. It does so by implementing two different Firestore classes, only one of which supports enablePersistence() and clearPersistence().

The new memory-only Firestore classes are exported as:

@firebase/firestore/memory
firebase/firestore/memory
firebase-firestore.memory.js

This PR also changes the integration tests to run against both versions of Firestore

schmidt-sebastian · 2020-02-08T06:39:04Z

@Feiyang1 Do you mind taking a look? If this is not moving in the direction of go/firebase-next, then we can also create Memory-only Firestore class and a PeristenceFirestore class that extends from it. That would provide more type safety.

Feiyang1 · 2020-02-12T21:59:07Z

packages/firestore/index.node.memory.ts

+import { FirebaseNamespace } from '@firebase/app-types';
+
+import { configureForFirebase } from './src/platform/config';
+import './src/platform_browser/browser_init';


Do you mean import './src/platform_node/node_init';?

Fixed. Thanks for catching.

Feiyang1 · 2020-02-12T22:49:02Z

packages/firestore/src/api/persistence.ts

+      if (!canFallback(error)) {
+        throw error;
+      }
+      console.warn(


use log from import * as log from '../util/log';?

Firestore doesn't support the log level "Warn".

Feiyang1 · 2020-02-12T23:00:46Z

packages/firestore/src/core/firestore_client.ts

+ *
+ * @returns A promise that will successfully resolve.
+ */
+export function newMemoryPersistence(


Just an organizational comment - Should this function live in src/api/persistence.ts ?

I tried to make it so the memory client doesn't import any persistence-related source (to make sure that my asserts in the rollup script work). Until we have size presubmits, I would like to keep these asserts to ensure that we don't accidentally include the wrong files.

To address your concern, I moved the factory-methods to their corresponding classes in ./local/persistence

Feiyang1 · 2020-02-13T00:32:46Z

packages/firestore/src/platform/persistence.ts

+ * Patches the Firestore prototype and provides the backing implementations for
+ * `Firestore.enablePersistence()` and `Firestore.clearPersistence()`.
+ */
+export function registerFirestorePersistence(


Instead of patching prototype, a cleaner way might be passing the persistence logics as a constructor parameter to Firestore class. Something like:

class Firestore { constructor( databaseIdOrApp: FirestoreDatabase | FirebaseApp, authProvider: Provider<FirebaseAuthInternalName>, private persistence: FirestorePersistence | null ) { ... } enablePersistence() { if ( this.persistence) { ... } else { throw new Error(PERSISTENCE_MISSING_ERROR_MSG); } } }

It can potentially allow us to export persistence logics and let developers decide whether or not to use persistence instead of us making multiple build targets.

we can also create Memory-only Firestore class and a PeristenceFirestore class that extends from it is also a good option that potentially allows developer to pick which one to use .

I updated the PR to use two different Firestore classes.

schmidt-sebastian

Revamped the PR to use two different Firestore classes. PTAL.

schmidt-sebastian · 2020-02-20T04:04:08Z

packages/firestore/src/platform/config.ts

- * the key 'firestore'. This is used for wrapped binary that exposes Firestore
- * as a goog module.
- */
-export function configureForStandalone(exportObject: {


FYI: This is part of #2646

schmidt-sebastian · 2020-02-28T20:49:13Z

PTAL @Feiyang1 :)

Feiyang1

LGTM, but I think the UX will be better if we make whether or not to use persistence a runtime choice instead of a compile time choice.

From my experience interacting with developers, many people don't know we provide different builds. Guides will need to be provided and they will likely live outside the reference docs, so I'm concerned many people will miss it. It also puts burden on developers that they need to dive into the build tool options to figure out how to pick the right build. As a result, we will likely see Github issues that ask why their build setup doesn't work, which can require a lot of time to debug.

I would advocate that we make the persistence/no persistence choice part of our official API, so it can be documented in reference docs and it will be very easy for developer to understand and use. Perhaps something like:

import * as firebase from 'firebase/app';
import { indexeddbPersistence } from 'firebase/firestore';

const app = firebase.initializeApp(...);
const firestore = app.firestore();

// enable persistence at runtime
firestore.enablePersistence(indexeddbPersistence, settings);

Feiyang1 · 2020-03-03T22:07:29Z

packages/firestore/rollup.config.js

+      json(),
+      terser(terserOptions)
+    ],
+    external: id => deps.some(dep => id === dep || id.startsWith(`${dep}/`))


use resolveMemoryExterns instead ?

schmidt-sebastian

Thanks for the review. Your suggestion seems like a good improvement and I will discuss with the Firestore team if we should adopt it.

I do have two questions though:

What should we do about the CDN builds? Should we only have a "firestore with everything build" and not provide the size savings for CDN users?
I believe the final API to run on persistence will look more like enablePersistence(firestore, settings) rather than firestore.enablePersistence(implementation, settings). If we roll this out via a breaking change, would it make more sense to use the firebase@next API directly?

schmidt-sebastian · 2020-03-03T23:04:46Z

packages/firestore/rollup.config.js

+      json(),
+      terser(terserOptions)
+    ],
+    external: id => deps.some(dep => id === dep || id.startsWith(`${dep}/`))


Feiyang1 · 2020-03-04T00:23:29Z

What should we do about the CDN builds? Should we only have a "firestore with everything build" and not provide the size savings for CDN users?

We can always make a special CDN build without persistence if there is sufficient demand, but I will be fine with just a firestore with everything build in CDN. In fact, we will only have [auth/app/...] with everything build in CDN after the modularization.

I believe the final API to run on persistence will look more like enablePersistence(firestore, settings) rather than firestore.enablePersistence(implementation, settings). If we roll this out via a breaking change, would it make more sense to use the firebase@next API directly?

SG. IIUC, I think it makes sense to make it part of firebase@next effort, but we don't have a proposal for Firestore yet. Perhaps we can create a proposal based on the persistence work?

schmidt-sebastian

Updated the PR to use composition. It probably makes more sense to compare against master than against the previous revision.

schmidt-sebastian · 2020-03-12T16:58:49Z

integration/firestore/package.json

-    "test": "echo 'Automated tests temporarily disabled, run `yarn test:manual` to execute these tests'",
-    "test:manual": "karma start --single-run",
-    "test:debug": "karma start --auto-watch --browsers Chrome"
+    "build:dependencies": "cd ../../ ; yarn build",


Fixed. Note that the main entry points should be test and related rules.

schmidt-sebastian · 2020-03-12T17:00:54Z

integration/firestore/package.json

-    "test:manual": "karma start --single-run",
-    "test:debug": "karma start --auto-watch --browsers Chrome"
+    "build:dependencies": "cd ../../ ; yarn build",
+    "build:persistence": "DISABLE_FIRESTORE_PERSISTENCE=false gulp compile-tests",


I have gone through multiple versions of this. I ended up with the negative flag since it allows me to treat the absence of the flag and its default value ("false") similarly. I, however, don't feel strongly and the check for the flag value is pretty isolated. Updated to use INCLUDE_FIRESTORE_PERSISTENCE.

schmidt-sebastian · 2020-03-12T17:07:38Z

integration/firestore/package.json

+    "build:dependencies": "cd ../../ ; yarn build",
+    "build:persistence": "DISABLE_FIRESTORE_PERSISTENCE=false gulp compile-tests",
+    "build:memory": "DISABLE_FIRESTORE_PERSISTENCE=true gulp compile-tests",
+    "test": "yarn build:memory; karma start --single-run; yarn build:persistence; karma start --single-run;",


Right now, gulp uses the same output folder for both modes. This means that the builds overwrite each other. We can probably use different output folders, but this would require some extra plumbing. Let me know if this is something that you want me to do.

schmidt-sebastian · 2020-03-12T17:09:23Z

packages/firebase/firestore/memory/index.ts

+ * limitations under the License.
+ */
+
+import '@firbease/firestore/memory';


This supports the firebase/firestore/memory import, which I believe we don't advocate for publicly.

@Feiyang1 Can you elaborate on the need for this?

schmidt-sebastian · 2020-03-12T17:10:54Z

packages/firebase/firestore/memory/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "firebase/firestore/memory",


This is modeled after the package.json files in firebase@next, but I believe you are right. @Feiyang1 should we add license/description fields?

schmidt-sebastian · 2020-03-12T22:54:51Z

packages/firestore/src/api/database.ts

@@ -382,65 +378,11 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
  }

  enablePersistence(settings?: firestore.PersistenceSettings): Promise<void> {


We could, but we would at the very least have to publish two different typings (and go through API review). Without duplicated typing, having these method here to throw an error seems like a better experience.

schmidt-sebastian · 2020-03-13T19:41:03Z

packages/firestore/src/api/database.ts

@@ -2678,7 +2624,7 @@ function applyFirestoreDataConverter<T>(

 // We're treating the variables as class names, so disable checking for lower
 // case variable names.
-export const PublicFirestore = makeConstructorPrivate(
+export const PublicMemoryFirestore = makeConstructorPrivate(


I overhauled the PR to use composition instead. enablePersistence() and clearPersistence() invoke a Persistence, and all private members are passed to the provider.

schmidt-sebastian · 2020-03-13T19:42:16Z

packages/firestore/src/api/persistence.ts

+ * A Firestore-implementation that provides support for `enablePersistence()`
+ * and `clearPersistence()`.
+ */
+export class PersistenceFirestore extends Firestore {


This is no longer applicable.

schmidt-sebastian · 2020-03-13T19:43:10Z

packages/firestore/src/local/indexeddb_persistence.ts

+ *
+ * @returns A Promise that resolves with the various persistence components.
+ */
+export const newIndexedDbPersistence: PersistenceFactory<IndexedDbPersistenceSettings> = async (


No longer applicable, but this style allows me to omit the return type (and I probably could have omitted the argument types as well).

schmidt-sebastian · 2020-03-13T19:51:45Z

packages/firestore/src/platform/config.ts

  (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
    new Component(
      'firestore',
      container => {
        const app = container.getProvider('app').getImmediate()!;
-        return new Firestore(app, container.getProvider('auth-internal'));
+        return new internalFirestore(


I am now able to use the constructor again, which reverts much of the changes here. The only difference is that I am now passing in a PersistenceProvider.

wilhuff

Very close to LGTM. A final suggestion on configureForFirebase and some nits/optional suggestions.

wilhuff · 2020-03-17T19:46:22Z

integration/firestore/package.json

+    "build:dependencies": "cd ../../ ; yarn build",
+    "build:persistence": "DISABLE_FIRESTORE_PERSISTENCE=false gulp compile-tests",
+    "build:memory": "DISABLE_FIRESTORE_PERSISTENCE=true gulp compile-tests",
+    "test": "yarn build:memory; karma start --single-run; yarn build:persistence; karma start --single-run;",


Ah. I see. It's certainly not something to tackle in this PR. If gulp were better about doing incremental builds this could be worthwhile, but I suspect it's not going to actually buy us much or possibly even anything.

wilhuff · 2020-03-17T19:49:51Z

packages/firebase/firestore/memory/index.ts

+ * limitations under the License.
+ */
+
+import '../../../firestore/dist/index.memory.esm';


It's still the case that looking at this file I can't tell why it exists, what bits of the build it interacts with, and whether or not I should use for anything when e.g. writing Firestore tests. I think this file is establishing the public interface to Firestore as seen when a user imports @firebase/firestore/memory, right? Comments that address these concerns would be helpful.

I added a short comment that explains this a little bit.

wilhuff · 2020-03-17T19:59:22Z

packages/firestore/src/api/database.ts

-    });
-    return deferred.promise;
+  async clearPersistence(): Promise<void> {
+    if (!this._persistenceProvider.isDurable) {


Is it possible to fold the throwing of these errors into the memory-only persistence provider?

Yes, that's a good clean up. I moved them to the MemoryPersistenceProvider.

wilhuff · 2020-03-17T21:20:39Z

packages/firestore/src/platform/config.ts

@@ -59,14 +60,27 @@ const firestoreNamespace = {

 /**
 * Configures Firestore as part of the Firebase SDK by calling registerService.
+ *
+ * @param firebase The FirebaseNamespace to register Firestore with
+ * @param persistenceProviderFactory A factory function that returns a


FWIW, what I was proposing was essentially to remove the Firestore instance creation from here altogether so that all the instances could be newed up together. Something like this here:

export function configureForFirebase( firebase: FirebaseNamespace, firestoreProvider: (app: FirebaseApp, auth: Provider<FirebaseAuthInternalName>) => Firestore ): void { (firebase as _FirebaseNamespace).INTERNAL.registerComponent( new Component( 'firestore', container => { const app = container.getProvider('app').getImmediate()!; const authProvider = container.getProvider('auth-internal'); return firestoreProvider(app, authProvider); }, ComponentType.PUBLIC ).setServiceProps(shallowCopy(firestoreNamespace)) ); }

For example, this is what index.memory.ts would look like:

export function registerFirestore(instance: FirebaseNamespace): void { configureForFirebase(instance, (app, authProvider) => { const persistenceProvider = new MemoryPersistenceProvider(); return new Firestore(app, authProvider, persistenceProvider); }); instance.registerVersion(name, version); }

The benefit of this arrangement is that we clearly separate how we plug into the component framework from the way we instantiate Firestore instances. If we move toward modularized initialization configureForFirebase won't have to know, and we won't have to refactor all the ways that Firestore instances are created at the same time.

I like what you have proposed and shamelessly updated the PR.

wilhuff · 2020-03-17T21:27:33Z

packages/firestore/src/local/persistence.ts

+   * True if the underlying implementation supports durable storage via
+   * `enablePersistence()`.
+   */
+  readonly isDurable: boolean;


Couldn't we have configure throw if the settings include enabling persistence but the provider doesn't actually support it?

That's how I ended up implementing the new error semantics for the MemoryPersistenceProvider.

wilhuff · 2020-03-17T21:29:54Z

packages/firestore/src/core/firestore_client.ts

+    persistenceResult: Deferred<void>
+  ): Promise<void> {
+    try {
+      await persistenceProvider.configure(


Since persistence provider's configure method is called during initializePersistence, consider calling this method initialize.

schmidt-sebastian

Addressed all feedback and also added an integration tests. This test surfaced an existing issue in canFallback: Instanceof checks don't seem to work for errors in JS (https://stackoverflow.com/questions/33870684/why-doesnt-instanceof-work-on-instances-of-error-subclasses-under-babel-node), so I had to change canFallback to rely on the error name instead.

schmidt-sebastian · 2020-03-18T04:21:09Z

packages/firebase/firestore/memory/index.ts

+ * limitations under the License.
+ */
+
+import '../../../firestore/dist/index.memory.esm';


I added a short comment that explains this a little bit.

schmidt-sebastian · 2020-03-18T04:25:05Z

packages/firebase/firestore/memory/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "firebase/firestore/memory",


Descriptions added to this package.json and top-level package.json.

schmidt-sebastian · 2020-03-18T04:25:38Z

packages/firestore/src/api/database.ts

-    });
-    return deferred.promise;
+  async clearPersistence(): Promise<void> {
+    if (!this._persistenceProvider.isDurable) {


Yes, that's a good clean up. I moved them to the MemoryPersistenceProvider.

schmidt-sebastian · 2020-03-18T04:25:48Z

packages/firestore/src/core/firestore_client.ts

+    persistenceResult: Deferred<void>
+  ): Promise<void> {
+    try {
+      await persistenceProvider.configure(


schmidt-sebastian · 2020-03-18T04:26:38Z

packages/firestore/src/local/persistence.ts

+   * True if the underlying implementation supports durable storage via
+   * `enablePersistence()`.
+   */
+  readonly isDurable: boolean;


That's how I ended up implementing the new error semantics for the MemoryPersistenceProvider.

schmidt-sebastian · 2020-03-18T04:27:12Z

packages/firestore/src/platform/config.ts

@@ -59,14 +60,27 @@ const firestoreNamespace = {

 /**
 * Configures Firestore as part of the Firebase SDK by calling registerService.
+ *
+ * @param firebase The FirebaseNamespace to register Firestore with
+ * @param persistenceProviderFactory A factory function that returns a


I like what you have proposed and shamelessly updated the PR.

wilhuff

LGTM

wilhuff · 2020-03-18T19:51:45Z

packages/firestore/rollup.config.js

 // size.
+//
+// See https://g3doc.corp.google.com/firebase/jscore/g3doc/contributing/builds.md


FYI, you can shorten this to http://g3doc/firebase/jscore/g3doc/contributing/builds.md if you wanted.

#2608 re-introduced mangling to the Node build, which wasn't mangled before. Since it uses CJS, the output is not minified very well. This reverts this change and makes sure the next release uses the same settings as the current release.

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners February 8, 2020 06:33

schmidt-sebastian force-pushed the mrschmidt/memorybuild branch from edae3ad to 3f89126 Compare February 8, 2020 06:36

schmidt-sebastian assigned Feiyang1 Feb 8, 2020

RFC: Memory-only Firestore build

860b649

schmidt-sebastian force-pushed the mrschmidt/memorybuild branch from 3f89126 to 860b649 Compare February 8, 2020 07:41

Feiyang1 reviewed Feb 13, 2020

View reviewed changes

schmidt-sebastian added 2 commits February 19, 2020 14:31

Merge

a4a8856

Review feedback

2080ba8

schmidt-sebastian force-pushed the mrschmidt/memorybuild branch from dc24a99 to 2080ba8 Compare February 20, 2020 00:34

schmidt-sebastian added 2 commits February 19, 2020 16:59

Merge

0ed63b3

Revert Object.assign changes

af52293

schmidt-sebastian changed the title ~~RFC: Memory-only Firestore build~~ Memory-only Firestore build Feb 20, 2020

schmidt-sebastian commented Feb 20, 2020

View reviewed changes

Update index.memory.ts

7121065

schmidt-sebastian commented Feb 20, 2020

View reviewed changes

schmidt-sebastian added 2 commits February 27, 2020 15:19

Merge

34c6f64

Lint fix

cce2ddd

schmidt-sebastian requested a review from andirayo as a code owner February 27, 2020 23:21

Fix output name

9b4ef91

Merge branch 'master' into mrschmidt/memorybuild

5d61740

Feiyang1 reviewed Mar 3, 2020

View reviewed changes

Use resolveMemoryExterns

9544744

schmidt-sebastian commented Mar 3, 2020

View reviewed changes

schmidt-sebastian added 2 commits March 6, 2020 10:37

Merge

e4a596c

Update to expose as @firebase/firestore/memory

df0b87c

Cleanup

de25f91

schmidt-sebastian commented Mar 13, 2020

View reviewed changes

Lint

07ff26e

schmidt-sebastian assigned Feiyang1 and wilhuff and unassigned schmidt-sebastian Mar 13, 2020

wilhuff reviewed Mar 17, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Mar 17, 2020

schmidt-sebastian force-pushed the mrschmidt/memorybuild branch from 4b22fbb to 397bddc Compare March 18, 2020 04:28

schmidt-sebastian commented Mar 18, 2020

View reviewed changes

schmidt-sebastian force-pushed the mrschmidt/memorybuild branch 2 times, most recently from 9949ce7 to cbfa9a1 Compare March 18, 2020 04:34

schmidt-sebastian added 2 commits March 17, 2020 21:53

Cleanup

6a6ec47

Merge

53a4d12

schmidt-sebastian force-pushed the mrschmidt/memorybuild branch from cbfa9a1 to 53a4d12 Compare March 18, 2020 04:54

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian and Feiyang1 Mar 18, 2020

wilhuff approved these changes Mar 18, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Mar 18, 2020

wilhuff added the api: firestore label Mar 18, 2020

Update rollup.config.js

df6a633

schmidt-sebastian merged commit a5d80d9 into master Mar 18, 2020

schmidt-sebastian deleted the mrschmidt/memorybuild branch March 18, 2020 20:35

schmidt-sebastian mentioned this pull request Mar 21, 2020

Remove mangling from Node build #2779

Merged

bernardodomingues-hotmart mentioned this pull request Mar 27, 2020

Invalid JSON @ firebase/firestore/memory/package.json #2810

Closed

firebase locked and limited conversation to collaborators Apr 18, 2020

		@@ -382,65 +378,11 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
		}

		enablePersistence(settings?: firestore.PersistenceSettings): Promise<void> {

Memory-only Firestore build #2608

Memory-only Firestore build #2608

Uh oh!

Conversation

schmidt-sebastian commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian commented Feb 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Feiyang1 Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Feb 28, 2020

Uh oh!

Feiyang1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Feiyang1 commented Mar 4, 2020

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

schmidt-sebastian commented Feb 8, 2020 •

edited

Loading

Feiyang1 Feb 13, 2020 •

edited

Loading

Feiyang1 left a comment •

edited

Loading