Add bundles to d.ts and rearrange bundles source code for building it as a separate module #4120

wu-hui · 2020-11-25T11:48:55Z

No description provided.

… as a separate module.

changeset-bot · 2020-11-25T11:48:58Z

⚠️ No Changeset found

Latest commit: 3f83901

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-11-25T12:14:29Z

Size Analysis Report

Affected Products

No changes between base commit (ddb7993) and head commit (82214fe).

Test Logs

Base (ddb7993d): https://github.com/firebase/firebase-js-sdk/actions/runs/383115622
Head (82214fe3): https://github.com/firebase/firebase-js-sdk/actions/runs/398554217

schmidt-sebastian · 2020-11-25T19:22:33Z

packages/firestore/src/api/bundle.ts

-  implements ApiLoadBundleTask, PromiseLike<ApiLoadBundleTaskProgress> {
-  private _progressObserver: PartialObserver<ApiLoadBundleTaskProgress> = {};
-  private _taskCompletionResolver = new Deferred<ApiLoadBundleTaskProgress>();
+  implements LoadBundleTask, PromiseLike<LoadBundleTaskProgress> {


Is LoadBundleTask implements LoadBundleTask intended?

Meant to be the interface from d.ts, fixed.

schmidt-sebastian · 2020-11-25T19:25:26Z

packages/firestore/src/api/bundle.ts

+): LoadBundleTask {
+  const resultTask = new LoadBundleTask();
+  // eslint-disable-next-line @typescript-eslint/no-floating-promises
+  firestoreClientLoadBundle(


You should drop the Promise from firestoreClientLoadBundle. It doesn't seem to be needed.

schmidt-sebastian · 2020-11-25T19:26:08Z

packages/firestore/src/core/bundle.ts

@@ -265,3 +257,113 @@ export class BundleLoader {
    return new BundleLoadResult({ ...this.progress }, changedDocuments);
  }
 }
+
+export async function firestoreClientLoadBundle(


As mentioned about, this doesn't need to be async.

schmidt-sebastian · 2020-11-25T19:27:30Z

packages/firestore/src/core/bundle.ts

+): Promise<void> {
+  const reader = createBundleReader(
+    data,
+    newSerializer((await client.getConfiguration()).databaseInfo.databaseId)


Can you pass in the databaseId or get it from FirestoreClient?

schmidt-sebastian · 2020-11-25T19:29:48Z

packages/firestore/src/core/bundle.ts

+  });
+}
+
+export function firestoreClientGetNamedQuery(


These two methods should live in firestore_client along with all their counterparts. In Android and iOS, they will be part of the FIrestoreClient class.

Done.

I leave the refactor for local_store and sync_engine alone, because they actually look good.

schmidt-sebastian · 2020-11-25T19:30:27Z

packages/firestore/src/local/local_store_bundle.ts

+} from '../model/collections';
+import { debugCast } from '../util/assert';
+
+import * as bundleProto from '../protos/firestore_bundle_proto';


No namespace imports please.

schmidt-sebastian · 2020-11-25T19:30:44Z

packages/firestore/test/integration/util/firebase_export.ts

@@ -28,9 +28,11 @@ import { FieldValue } from '../../../src/compat/field_value';
 import { FieldPath } from '../../../src/api/field_path';
 import { Timestamp } from '../../../src/api/timestamp';
 import { Blob } from '../../../src/api/blob';
+null;


Please remove

schmidt-sebastian · 2020-11-25T19:31:18Z

packages/firestore/test/integration/util/firebase_export.ts

 import { GeoPoint } from '../../../src/api/geo_point';
 import { FirebaseApp } from '@firebase/app-types';
 import { Firestore } from '../../../src/api/database';
+import '../../../index.bundle';


Should we add a comment here?

packages/firestore/index.bundle.ts

schmidt-sebastian · 2020-11-30T22:17:57Z

packages/firestore/index.bundle.ts

+    return namedQuery(this as any, queryName);
+  };
+
+  //TODO: add loadBundle and namedQuery to the firestore namespace


Please assign TODO to yourself or to a bug

This was added by Fei, I actually don't know what he meant by this. Added my name anyways.

schmidt-sebastian · 2020-11-30T22:19:41Z

packages/firestore/src/core/bundle.ts

+ * @param bundleReader Bundle to load into the SDK.
+ * @param task LoadBundleTask used to update the loading progress to public API.
+ */
+export function syncEngineLoadBundle(


This should live in SyncEngine as it uses the private SyncEngine types (SyncEngineImpl should not be used outside of sync_engine.ts)

schmidt-sebastian · 2020-11-30T22:19:55Z

packages/firestore/src/core/sync_engine.ts

@@ -209,7 +200,7 @@ export interface SyncEngine {
 * This is useful to implement optional features (like bundles) in free
 * functions, such that they are tree-shakeable.
 */
-class SyncEngineImpl implements SyncEngine {
+export class SyncEngineImpl implements SyncEngine {


Please don't export this class, as it leaks all the public members that are only public for consumers in this file.

schmidt-sebastian · 2020-11-30T22:21:58Z

packages/firestore/src/local/local_store.ts

@@ -183,7 +174,7 @@ export interface LocalStore {
 * This is useful to implement optional features (like bundles) in free
 * functions, such that they are tree-shakeable.
 */
-class LocalStoreImpl implements LocalStore {
+export class LocalStoreImpl implements LocalStore {


Same comments regarding the changes here as in SyncEngine. In general, since we don't seem to need the source re-arrangement anymore (with the prebuilt), we should move the bundle code back to where it was before.

schmidt-sebastian · 2020-11-30T22:22:30Z

packages/firestore/src/local/local_store_bundle.ts

+ * LocalDocuments are re-calculated if there are remaining mutations in the
+ * queue.
+ */
+export async function applyBundleDocuments(


Would prefer to have this back in local_store.ts

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

schmidt-sebastian · 2020-12-01T02:51:56Z

packages/firestore/index.bundle.ts

+import { loadBundle, namedQuery } from './src/api/bundle';
+
+/**
+ * Registers the memory-only Firestore build with the components framework.


Comment needs updating.

schmidt-sebastian · 2020-12-01T02:52:22Z

packages/firestore/index.bundle.ts

+    return namedQuery(this, queryName);
+  };
+
+  //TODO(wuandy): add loadBundle and namedQuery to the firestore namespace


If we both don't know what this refers to, then we might as well remove it.

packages/firestore/src/core/bundle_types.ts

schmidt-sebastian · 2020-12-01T02:54:18Z

packages/firestore/src/core/bundle_types.ts

+  readonly id: string;
+  readonly version: number;


Ideally, this would be commented as well as it will be used to generate the firestore-exp docs (once you merge master)

schmidt-sebastian · 2020-12-01T02:54:41Z

packages/firestore/src/core/bundle_types.ts

+  readonly name: string;
+  readonly query: Query;


Same applies here - we should add JSDoc.

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

* Add bundles to d.ts and rearrange bundles source code for building it as a separate module (#4120) * Add bundles to d.ts and rearrange bundles source code for building it as a separate module. * Roll bundle with prebuilt (#4128) Build firestore sdks with prebuilt to support bundle as a prototype patched feature. * Add JSDoc for bundles (#4155) Add JSDoc for bundles. * Create changeset * Update old-lobsters-pull.md * A few fixes for Firestore bundle sdk builds (#4177) * Actually creates firebase/firestore/bundle package.json * Fix standard node/browser builds for bundles * Make it release from remote branch. * Add bundle to firebase complete build and fix missing proto for memory build. * Make rollup.config.js more organized. * Revert "Make it release from remote branch." This reverts commit 2c91fd1. * 2017 -> 2020 * Update comment. * Address comments * Update .changeset/old-lobsters-pull.md Co-authored-by: Sebastian Schmidt <mrschmidt@google.com> * Update .changeset/old-lobsters-pull.md Co-authored-by: Sebastian Schmidt <mrschmidt@google.com> * Add toc. * Remove webpack change. Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

Add bundles to d.ts and rearrange bundles source code for building it…

e11f1fe

… as a separate module.

wu-hui requested a review from schmidt-sebastian November 25, 2020 11:50

wu-hui assigned schmidt-sebastian Nov 25, 2020

wu-hui changed the base branch from master to wuandy/ReleaseBundles November 25, 2020 11:52

schmidt-sebastian reviewed Nov 25, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Nov 25, 2020

wu-hui added 3 commits November 26, 2020 09:40

Address comments minus revert refactoring.

79b1ed2

Address comments and restore some code structure.

6000ea1

Keep bundle_types.ts, otherwise minified test breaks.

326bb02

schmidt-sebastian reviewed Nov 30, 2020

View reviewed changes

schmidt-sebastian removed their assignment Nov 30, 2020

wu-hui and others added 3 commits November 30, 2020 20:19

Update packages/firestore/index.bundle.ts

fe8ac37

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

Move functions back to sync_engine and local_store.ts

0b1a4c7

Remove exports

686d333

wu-hui assigned schmidt-sebastian and unassigned wu-hui Dec 1, 2020

schmidt-sebastian approved these changes Dec 1, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Dec 1, 2020

wu-hui and others added 2 commits December 3, 2020 09:16

Update packages/firestore/src/core/bundle_types.ts

571ddcf

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

More accurate and complete comments.

3f83901

wu-hui merged commit cc8dd9c into wuandy/ReleaseBundles Dec 3, 2020

firebase locked and limited conversation to collaborators Jan 3, 2021

Add bundles to d.ts and rearrange bundles source code for building it as a separate module #4120

Add bundles to d.ts and rearrange bundles source code for building it as a separate module #4120

Conversation

wu-hui commented Nov 25, 2020

changeset-bot bot commented Nov 25, 2020 • edited Loading

⚠️ No Changeset found

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

Size Analysis Report

Affected Products

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

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

changeset-bot bot commented Nov 25, 2020 •

edited

Loading

google-oss-bot commented Nov 25, 2020 •

edited

Loading