Compat Layer for Firestore #4003

schmidt-sebastian · 2020-10-27T23:25:58Z

This PR replaces the Firestore class from the main SDK with a Compat class that delegates most simple calls to firestore-exp (e.g. enableNetwork, waitForPendingWrites) and drops the no longer needed FirestoreCompat interface. The OR doesn't yet replace WriteBatch, runTransaction, DocumentReference or CollectionReference and instead uses the legacy implementations.

Hidden in these changes is also unwrapping code that recognizes the FieldValue Compat types and unwraps them so that they are recognized by the UserDataConverter.

There are also a couple of test updates in this PR. Most are due to the unfortunate fact that we are losing test coverage for firestore-exp's WriteBatch, runTransaction, DocumentReference and CollectionReference while they are undergoing migration.

changeset-bot · 2020-10-27T23:26:01Z

⚠️ No Changeset found

Latest commit: 869d59c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, 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 · 2020-10-28T00:02:58Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`39847b8`)	Head (`44508f5`)	Diff
browser	241 kB	241 kB	-211 B (-0.1%)
esm2017	190 kB	190 kB	-220 B (-0.1%)
main	475 kB	476 kB	+459 B (+0.1%)
module	241 kB	241 kB	-211 B (-0.1%)
react-native	190 kB	190 kB	-219 B (-0.1%)

`@firebase/firestore/exp`

Type	Base (`39847b8`)	Head (`44508f5`)	Diff
browser	189 kB	190 kB	+1.04 kB (+0.6%)
main	476 kB	479 kB	+2.69 kB (+0.6%)
module	189 kB	190 kB	+1.04 kB (+0.6%)
react-native	189 kB	190 kB	+1.04 kB (+0.5%)

`@firebase/firestore/lite`

Type	Base (`39847b8`)	Head (`44508f5`)	Diff
browser	62.9 kB	63.9 kB	+1.00 kB (+1.6%)
main	139 kB	141 kB	+2.60 kB (+1.9%)
module	62.9 kB	63.9 kB	+1.00 kB (+1.6%)
react-native	63.1 kB	64.1 kB	+998 B (+1.6%)

`@firebase/firestore/memory`

Type	Base (`39847b8`)	Head (`44508f5`)	Diff
browser	177 kB	178 kB	+191 B (+0.1%)
esm2017	139 kB	139 kB	+189 B (+0.1%)
main	344 kB	345 kB	+1.16 kB (+0.3%)
module	177 kB	178 kB	+191 B (+0.1%)
react-native	139 kB	140 kB	+203 B (+0.1%)

`firebase`

Type	Base (`39847b8`)	Head (`44508f5`)	Diff
firebase-firestore.js	280 kB	283 kB	+3.68 kB (+1.3%)
firebase-firestore.memory.js	217 kB	222 kB	+4.11 kB (+1.9%)
firebase.js	822 kB	826 kB	+3.15 kB (+0.4%)

Test Logs

Base (39847b8e): https://github.com/firebase/firebase-js-sdk/actions/runs/346154034
Head (44508f51): https://github.com/firebase/firebase-js-sdk/actions/runs/346407427

schmidt-sebastian · 2020-11-02T17:38:20Z

packages/firestore/package.json

@@ -79,6 +79,7 @@
  },
  "devDependencies": {
    "@firebase/app": "0.6.12",
+    "@firebase/app-compat": "0.x",


@Feiyang1 Can you confirm that app-compat is fine as a test-dependency? Without it, the browser tests fail to build.

wilhuff · 2020-11-02T18:02:08Z

packages/firestore/lite/src/api/database.ts

+ * Options that can be provided in the Firestore constructor when not using
+ * Firebase (aka standalone mode).
+ */
+export interface FirestoreDatabase {


On other platforms this thing is a DatabaseId. Is there any reason not to promote DatabaseId to this common layer and just use that type throughout?

Alternatively, rather than inventing new names for existing concepts to just to make compatibility interfaces, it seems like we could just stick with the Compat suffix for this kind of thing. This way we can immediately recognize this thing as essentially the same thing as a DatabaseId without having to go read about what a FirestoreDatabase is.

This should only be used for the console build, where the FirestorewDatabase name is already used. It is slightly different from the existing DatabaseId in that the database name is optional.

I was able to move the conversion into index.console.ts, which narrows down the scope here quite a bit. It also allows us to a proper instanceof check in the Firestore constructor.

wilhuff · 2020-11-02T18:02:59Z

packages/firestore/exp/src/api/database.ts

  readonly _queue = new AsyncQueue();
  readonly _persistenceKey: string;

  _firestoreClient: FirestoreClient | undefined;

  constructor(
-    app: FirebaseApp,
+    app: FirestoreDatabase | FirebaseApp,


Should this parameter name be databaseIdOrApp to match other similar ones?

wilhuff · 2020-11-02T18:09:59Z

packages/firestore/index.node.ts

@@ -19,6 +19,7 @@ import { FirebaseNamespace } from '@firebase/app-types';

 import { Firestore, IndexedDbPersistenceProvider } from './src/api/database';
 import { configureForFirebase } from './src/config';
+import { FirebaseFirestore as ExpFirebaseFirestore } from './exp/src/api/database';


In some cases you're using "exp" and "lite" as suffixes and sometimes as prefixes. Sometimes the root is abbreviated "Firestore" and sometimes it's "FirebaseFirestore". This is messy.

Can we just name these classes distinctly at all the different layers so that we don't need to rename when importing? This will make code searching easier because searching for ExpFirebaseFirestore is going to find the actual class definition, not the import rename lines.

I think we actually need to rename the one renaming Firestore class (the legacy class) to FirebaseFirestore as well. The problem is that we want to use the class name in the error messages (see https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/util/input_validation.ts#L179). If we rename the classes in the source, the new name leaks via the constructor.name property and the check will no longer print the more helpful error message.

So I think the way forward should be:

Rename the Firestore class in api/database.ts to FirebaseFirestore.

Settle on a prefix or suffix notation.

We could solve the input validation class name problem by stripping known class-name prefixes for the different layers from the start of the class name we put in error messages.

Agreed, we should rename classic Firestore to match the pattern in the other layers.

Generally when classes are refinements of some other class, the refinement is first (think Exception -> IOException, etc). However, FirebaseFirestoreLite reads OK, (but sounds like a beer name). FirebaseFirestoreClassic reads fine too (but sounds like a soda name).

On the other hand, you used "exp" as a prefix here and nearby. You went with a "public" prefix elsewhere in this PR. LiteFirebaseFirestore and ClassicFirebaseFirestore seem to read fine. On balance that probably favors using prefixes, but if you have a preference that goes the other way I can see it. I'm more concerned about consistency than anything.

This PR is already huge so there's no need to address any class renaming now (though I'd prefer that the import renaming end up consistent, if possible).

I will send a follow-up PR that addresses these issues shortly.

Renaming the internals breaks the minifier since the renamed symbols are not considered part of the public API. This doesn't affect the "Public" rename, so I will still go ahead with that.

wilhuff · 2020-11-02T18:16:47Z

packages/firestore/src/api/database.ts

+import {
+  CollectionReference as PublicCollectionReference,
+  DocumentChange as PublicDocumentChange,
+  DocumentChangeType,


Optional: consider just prefixing all of these types with "Public" so that the origin is obvious.

Alternatively: since I'm pushing you to name classes in these layers distinctly, maybe name elements in here with a "Classic" prefix?

I can do this (prefer the "Classic" prefix), but would like to do as a follow up since this PR is already pretty large.

wilhuff · 2020-11-02T19:29:14Z

packages/firestore/src/api/user_data_reader.ts

+
+      // For Compat types, we have to "extract" the underlying types before
+      // performing validation.
+      if (value instanceof Compat) {


I'd like to see us get to a place where we're not wrapping and unwrapping these types but instead just sharing a single definition for all the variations. Obviously this isn't a topic for this PR and I'm not advocating changes right now.

I don't quite see how this would work, but would love to get there. Maybe we can talk about this more, since I would need some guidance on how to achieve this goal.

Let's chat offline; no need to address in this PR anyway.

schmidt-sebastian · 2020-11-03T19:55:17Z

Punted on the renames after all, since it makes more sense to just do a batch-rename once this is merged.

wilhuff

LGTM

github-actions · 2020-11-04T20:43:48Z

Changeset File Check ✅

No modified packages are missing from the changeset file.
No changeset formatting errors detected.

google-oss-bot · 2020-11-04T21:01:49Z

Size Analysis Report

Affected Products

No changes between base commit (39847b8) and head commit (44508f5).

Test Logs

Base (39847b8e): https://github.com/firebase/firebase-js-sdk/actions/runs/346154034
Head (44508f51): https://github.com/firebase/firebase-js-sdk/actions/runs/346407427

* Rolls a node app building bundles. * Build bundle files for given list of project IDs. * Build the bundle json map and save it for integration tests. * Add emulator_settings.ts to gulp * Move bundle.test.ts to api/ * Bundles passes all tests and expose in classic API * Add CI project ID to bundles. * Adhoc string replacement and length re-calculation * Fix lint errors. * Delete old changes from make node app * Address comments * Update yarn.lock for release (#3998) Temp fix where database version is bumped before firebase-admin can update deps * Manually prepares the bundle strings. * Update API * Update config.ts * Use Chrome for karma debugging (#4007) * Cache emulator between runs (#3956) * Remote Config Modularization (#3975) * rc exp init * Add apis * register rc exp * implement funcitonal APIs * fix tests * build rc exp * add api-extractor to rc types * cast directly witout function * delete changelog for rc exp * add code owners to rc exp * update dep version * Remove AuthErrorCode from core export (#4013) * Remove AuthErrorCode from core export * Api * Update config.ts to remove bundles * adds a root changelog (#4009) * adds a root changelog * Update CHANGELOG.md Co-authored-by: Feiyang <[email protected]> * Update dependency typescript to v4.0.5 (#3846) Co-authored-by: Renovate Bot <[email protected]> * Update dependency karma-firefox-launcher to v2 (#3987) Co-authored-by: Renovate Bot <[email protected]> * Update dependency google-closure-library to v20200830 (#3765) * Update dependency google-closure-library to v20200830 * Replace goog.isArray with Array.isArray https://github.com/google/closure-library/releases/tag/v20200628 Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Alex Volkovitsky <[email protected]> * exclude remote config exp packages in changeset (#4014) * Set 1s timeout for onBackgroundMessage Hook (#3780) * await onBackgroundMessage hook * Create fluffy-panthers-hide.md * block in onPush to let onBackgroundMessage to execute * polish wording * Update changeset to be more specific * Update fluffy-panthers-hide.md * Clarify PR is about a bug fix * Update fluffy-panthers-hide.md * A whole bunch of things to bring auth (compat) to parity (#3970) * Handle anonymous auth re-login edge case * Formatting * Initial fixes * Fix redirect * clean up additional user info * Formatting * PR feedback * Fix some tests * Fix tests & write some new ones * Fix broken build * Formatting * Formatting * PR feedback * Formatting * PR feedback Co-authored-by: avolkovi <[email protected]> * Add withFunctionsTriggersDisabled method to rules-unit-testing (#3928) * Update integration tests to use free functions * Functions compat package (#3739) * Add free functions to exports * Fix to avoid false failures on changeset checker (#4012) * Add changeset for Firestore (#4030) * Update functions-compat dep version and fix changeset script error (#4032) * Update integration tests. Minified tests fail. * Bump node memory limit for all test CI (#4035) * Compat Layer for Firestore (#4003) * Rename all public API types to PublicX (#4039) * Update all non-major dependencies (#3953) Co-authored-by: Renovate Bot <[email protected]> * Version Packages (#4033) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Set up Storage modularization (#3499) Refactor storage for modularization. * Free functions removed from exp database.ts Co-authored-by: Christina Holland <[email protected]> Co-authored-by: Sebastian Schmidt <[email protected]> Co-authored-by: Sam Stern <[email protected]> Co-authored-by: Feiyang <[email protected]> Co-authored-by: Sam Horlbeck Olsen <[email protected]> Co-authored-by: Dimitri Mitropoulos <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Alex Volkovitsky <[email protected]> Co-authored-by: Kai Wu <[email protected]> Co-authored-by: Google Open Source Bot <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

schmidt-sebastian force-pushed the mrschmidt/firestore-compat-v2 branch from dd54e24 to 557e0cb Compare October 27, 2020 23:34

schmidt-sebastian force-pushed the mrschmidt/firestore-compat-v2 branch from 6f79955 to 0f88e5b Compare October 29, 2020 23:06

Compat Layer for Firestore

8305a4f

schmidt-sebastian force-pushed the mrschmidt/firestore-compat-v2 branch from b55cde8 to 8305a4f Compare November 2, 2020 17:44

schmidt-sebastian commented Nov 2, 2020

View reviewed changes

schmidt-sebastian requested a review from wilhuff November 2, 2020 17:45

schmidt-sebastian assigned wilhuff Nov 2, 2020

schmidt-sebastian added the api: firestore label Nov 2, 2020

wilhuff reviewed Nov 2, 2020

View reviewed changes

Address some of the feedback

a91f02d

schmidt-sebastian marked this pull request as ready for review November 2, 2020 23:53

Lint

07dfa9f

firebase deleted a comment from google-oss-bot Nov 3, 2020

wilhuff approved these changes Nov 4, 2020

View reviewed changes

Fix test build

0fb56e1

schmidt-sebastian force-pushed the mrschmidt/firestore-compat-v2 branch from 2ca34ce to 0fb56e1 Compare November 4, 2020 21:34

Add app-compat hack (#4036)

36fb4c7

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners November 4, 2020 21:35

Add buildAppCompat flag to Firestore test workflows (#4037)

869d59c

hsubox76 approved these changes Nov 4, 2020

View reviewed changes

schmidt-sebastian merged commit 9d424f5 into master Nov 4, 2020

schmidt-sebastian deleted the mrschmidt/firestore-compat-v2 branch November 9, 2020 22:36

firebase locked and limited conversation to collaborators Dec 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compat Layer for Firestore #4003

Compat Layer for Firestore #4003

schmidt-sebastian commented Oct 27, 2020 •

edited

Loading

changeset-bot bot commented Oct 27, 2020 •

edited

Loading

google-oss-bot commented Oct 28, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

schmidt-sebastian Nov 2, 2020

wilhuff Nov 2, 2020

schmidt-sebastian Nov 2, 2020

wilhuff Nov 3, 2020

wilhuff Nov 2, 2020

schmidt-sebastian Nov 2, 2020

wilhuff Nov 2, 2020

schmidt-sebastian Nov 2, 2020

wilhuff Nov 3, 2020

schmidt-sebastian Nov 3, 2020

schmidt-sebastian Nov 3, 2020

wilhuff Nov 2, 2020

schmidt-sebastian Nov 2, 2020

wilhuff Nov 3, 2020

wilhuff Nov 2, 2020

schmidt-sebastian Nov 2, 2020

wilhuff Nov 3, 2020

schmidt-sebastian commented Nov 3, 2020

wilhuff left a comment

github-actions bot commented Nov 4, 2020 •

edited

Loading

google-oss-bot commented Nov 4, 2020 •

edited

Loading

Compat Layer for Firestore #4003

Compat Layer for Firestore #4003

Conversation

schmidt-sebastian commented Oct 27, 2020 • edited Loading

changeset-bot bot commented Oct 27, 2020 • edited Loading

⚠️ No Changeset found

google-oss-bot commented Oct 28, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/exp

@firebase/firestore/lite

@firebase/firestore/memory

firebase

Test Logs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Nov 3, 2020

wilhuff left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 4, 2020 • edited Loading

Changeset File Check ✅

google-oss-bot commented Nov 4, 2020 • edited Loading

Size Analysis Report

Affected Products

Test Logs

schmidt-sebastian commented Oct 27, 2020 •

edited

Loading

changeset-bot bot commented Oct 27, 2020 •

edited

Loading

google-oss-bot commented Oct 28, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

github-actions bot commented Nov 4, 2020 •

edited

Loading

google-oss-bot commented Nov 4, 2020 •

edited

Loading