Replace Platform injection with source preprocessor #3206

schmidt-sebastian · 2020-06-11T06:10:25Z

So this will either get my commit access revoked or result in lot of external job offers.

One of the problems with our client initialization is that we use a single Platform class to inject all platform specific dependencies. In the Lite and the Tree-Shakeable SDK this means that pulling in any code at all will bring in the platform specific dependencies.

This PR removes the Platform class and instead replaces each API with a top-level function. It then uses rollup and webpack plugins to replace the a generic platform with the target platform.
Due to some dependency conflict with ts-node, I had to extract out "formatJSON" and "randomBytes" into separate files.

google-oss-bot · 2020-06-11T20:00:51Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`338dbfa`)	Head (`dfa111f`)	Diff
browser	250 kB	249 kB	-1.26 kB (-0.5%)
esm2017	195 kB	194 kB	-665 B (-0.3%)
main	493 kB	491 kB	-1.35 kB (-0.3%)
module	248 kB	246 kB	-1.23 kB (-0.5%)
react-native	195 kB	194 kB	-886 B (-0.5%)

@firebase/firestore/lite

Type Base (338dbfa) Head (dfa111f) Diff

main 140 kB 139 kB -967 B (-0.7%)

`@firebase/firestore/memory`

Type	Base (`338dbfa`)	Head (`dfa111f`)	Diff
browser	187 kB	186 kB	-1.06 kB (-0.6%)
esm2017	146 kB	145 kB	-477 B (-0.3%)
main	363 kB	361 kB	-1.36 kB (-0.4%)
module	185 kB	184 kB	-1.02 kB (-0.5%)
react-native	146 kB	145 kB	-697 B (-0.5%)

`firebase`

Click to show 13 binary size changes.

Type	Base (`338dbfa`)	Head (`dfa111f`)	Diff
firebase-analytics.js	26.6 kB	26.6 kB	+3 B (+0.0%)
firebase-auth.js	173 kB	173 kB	+242 B (+0.1%)
firebase-database.js	187 kB	187 kB	+547 B (+0.3%)
firebase-firestore.js	287 kB	287 kB	-135 B (-0.0%)
firebase-firestore.memory.js	226 kB	226 kB	-194 B (-0.1%)
firebase-functions.js	9.60 kB	9.60 kB	-1 B (-0.0%)
firebase-installations.js	19.2 kB	19.2 kB	+2 B (+0.0%)
firebase-performance-standalone.es2017.js	72.0 kB	72.0 kB	+13 B (+0.0%)
firebase-performance-standalone.js	47.2 kB	47.4 kB	+191 B (+0.4%)
firebase-performance.js	37.6 kB	37.8 kB	+203 B (+0.5%)
firebase-remote-config.js	37.0 kB	37.0 kB	+2 B (+0.0%)
firebase-storage.js	40.8 kB	40.9 kB	+73 B (+0.2%)
firebase.js	819 kB	820 kB	+830 B (+0.1%)

Test Logs

Base (338dbfad): https://github.com/firebase/firebase-js-sdk/actions/runs/141276984
Head (dfa111fc): https://github.com/firebase/firebase-js-sdk/actions/runs/141280100

schmidt-sebastian · 2020-06-12T15:50:37Z

I just realized that we might also be able to do this if we publish an internal @firebase/firestore/platform package that exposes different builds for node, browser and react-native. I can explore that next week if that seems cleaner (but it seems a bit tricky since @firebase/firestore/platform would have a lot of dependencies on the internal Firestore code).

Update: I don't think this is possible due to the sheer number of internal dependencies that @firebase/firestore/platform would have.

wilhuff

LGTM with a few nits.

This isn't a crazy idea at all--it's very similar to the link-time substitution we do for platform-dependent code in the iOS codebase.

One concern: should the CDN builds be uber builds that work anywhere? As it stands it seems like they become forcibly browser. This, in turn, means that users have to take on the complexity of choosing the right build for their platform. Would we want to make it so that if you take the CDN build it will run anywhere, to keep things simple for hackathons and other get-up-and-running-quickly scenarios where you may not want to invest in setting up a bundler?

wilhuff · 2020-06-16T01:59:11Z

packages/firestore/src/platform/platform.ts

+export function loadConnection(
+  databaseInfo: DatabaseInfo
+): Promise<Connection> {
+  return isNode()


Rather than duplicating this isNode ? yes : no block in every function, is it possible to just assign a global with the platform module once? Then you'd end up with something like this here:

return getPlatform().loadConnection(databaseInfo);

Perhaps this isn't possible because there's no longer an interface that describes what every module has to conform to, in which case I'm not sure it's worth adding that interface back, but it's worth a thought.

I don't think this would be possible unless we define an interface as an intermediate layer. It did however dawn on me that this file is never used in browser builds. I ended up replacing the isNode check with an assert.

wilhuff · 2020-06-16T17:02:14Z

config/webpack.test.js

@@ -18,6 +18,9 @@
 const path = require('path');
 const webpack = require('webpack');

+/** A regular expression used to replace Firestore' platform specific modules. */


Missing 's' in "Firestore's"?

Could you expand this comment to indicate which files this is intended to match (e.g. packages/firestore/src/platform)?

Also you're matching specific files in here but not all files, which is confusing for constructing a mental model of how the platform files are handled. It would be easier to reason about this if you could just have a blanket rule that everything in platform is replaced.

I expanded the comment and changed the RegExp to match all files. This is cleaner, but I initially wanted to keep it more restricted because I am afraid that it will start matching files in other platforms. Unfortunately, I haven't found a way to restrict it to just "packages/firestore" since the paths that are used are the relative import paths we specify in our sources.

As a general heads up - I debated getting rid of the platform.ts file before, but kept it as is during the first review. I have since moved all implementations to specific "category" files (base64.ts, serializer.ts, dom.ts, etc). These seems cleaner that having two files that are outliers.

wilhuff · 2020-06-16T17:08:50Z

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

-        );
-        this._datastoreDeferred.resolve(datastore);
-      });
+    loadConnection(databaseInfo).then(connection => {


What value is there in distinguishing that we "load" the connection where the other things are just new? Seems like this could benewConnection.

Updated to newConnection.

wilhuff · 2020-06-16T19:18:57Z

packages/firestore/src/platform_node/node_platform.ts


-    return randomBytes(nBytes);
-  }
+/** True if and only if the Base64 conversion functions are available. */


This seems related to the other base64 methods--maybe group these together?

They are now group in the "base64" module.

wilhuff · 2020-06-16T19:19:46Z

packages/firestore/src/platform_rn/rn_platform.ts

+// ReactNative-specific platform implementation that provides its own Base64
+// encoding.
+// The exports in this class must match the exports in '../platform/platform' as
+// they are bundled with the browser build during the Rollup build.


These aren't bundled with the browser build--it's the react native, build, right?

That's correct. The comment has been removed.

wilhuff · 2020-06-16T19:23:06Z

packages/firestore/src/util/types.ts

@@ -47,3 +47,18 @@ export function isSafeInteger(value: unknown): boolean {
    value >= Number.MIN_SAFE_INTEGER
  );
 }
+
+/** The subset of the Window interface used by the SDK. */


Is Window universally available? I thought it was specific to the Browser? If so, minor nit: this is a subset of the browser's Window class. Same nit below.

Feiyang1

Very nice! just a few minor nits.

Feiyang1 · 2020-06-16T22:02:35Z

packages/firestore/src/platform/format_json.ts

+import * as browser from '../platform_browser/browser_format_json';
+import { isNode } from '@firebase/util';
+
+// This file provides additional platform specific APIs. It acts an add on to


missing "as" after acts

Moved this to src/platform/README.md, which contains different text.

Feiyang1 · 2020-06-16T22:03:37Z

packages/firestore/src/platform/platform.ts


-  /** Formats an object as a JSON string, suitable for logging. */
-  formatJSON(value: unknown): string;
+// NOTE: This class is replaced in Firestore release builds by either


s/class/module

Addressed in README.md

Feiyang1 · 2020-06-16T22:04:46Z

packages/firestore/src/platform/random_bytes.ts

+import * as browser from '../platform_browser/browser_random_bytes';
+import { isNode } from '@firebase/util';
+
+// This file provides additional platform specific APIs. It acts an add on to


missing "as" after acts

wilhuff

I'm confused. How do browser-based tests work? Do they have to do the full rollup build to work? That seems unfortunately because it prevents us from doing a live reload style of development.

Also, you didn't really address the question I asked at the top of the last review. This seems to be pushing us in a direction there's no way to make a build that "just works", regardless of the platform. I recognize that it's useful to produce a slimmed down web-only build, but there's also value in not having to choose and being able to get started quickly regardless of environment.

wilhuff · 2020-06-17T18:11:22Z

packages/firestore/src/platform/README.md

@@ -0,0 +1,4 @@
+The modules in this directory provide implementations for platform-specific features that are replaced at build time


Nit: use the same line length as our sources?

schmidt-sebastian

I updated the "generic" platform files to bring back support for Node, RN and Browsers. With that being soid, the only environment these will actually run in is Node, unless someone else builds their own build pipeline that consumes our source and not our artifacts.
For what it is worth, the generic components only compile under Node, since @grpc/grpc-js causes bundling errors when included in the Browser builds.

I did verify that Karma's auto-watch still works as is, picking up changes in our normal sources as well as in the Platform files.

schmidt-sebastian · 2020-06-17T19:58:26Z

packages/firestore/src/platform/README.md

@@ -0,0 +1,4 @@
+The modules in this directory provide implementations for platform-specific features that are replaced at build time


wilhuff

Pretty much LGTM

wilhuff · 2020-06-19T18:43:39Z

packages/firestore/src/platform/connection.ts

@@ -0,0 +1,53 @@
+/**
+ * @license
+ * Copyright 2017 Google LLC


2020? Here and throughout.

wilhuff · 2020-06-19T18:45:46Z

packages/firestore/src/platform/connection.ts

+ */
+
+import { isNode, isReactNative } from '@firebase/util';
+import {


This is pretty hefty amount of code to make all these distinct imports. If this is what it takes I guess this is what we're stuck with, but it doesn't seem like this file needs to be tree-shakeable so whole module imports seem like they'd work and would cost way less.

It is pretty ugly indeed. I reverted back to namespace imports.

yuchenshi

Approval for packages/testing for that one line change from Google Inc. to Google LLC. Sure, why not.

wilhuff

LGTM

schmidt-sebastian force-pushed the mrschmidt/treeshakeableplatform branch from 5f799e6 to 9fb9e1f Compare June 11, 2020 19:51

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners June 11, 2020 19:51

Replace Platform injection with source preprocessor

a7dc96f

schmidt-sebastian force-pushed the mrschmidt/treeshakeableplatform branch from f3296f5 to a7dc96f Compare June 11, 2020 21:13

schmidt-sebastian changed the title ~~Untangle Platform~~ Replace Platform injection with source preprocessor Jun 11, 2020

schmidt-sebastian assigned Feiyang1 and wilhuff Jun 11, 2020

schmidt-sebastian added the api: firestore label Jun 11, 2020

schmidt-sebastian added 4 commits June 11, 2020 19:10

Use stricter RegExp

69e4ad2

Use Webbpack Replacement Plugin

654cafc

Combine RegExps

f714da4

Reduce yarn.lock diff

060f193

wilhuff reviewed Jun 16, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jun 16, 2020

Feiyang1 approved these changes Jun 16, 2020

View reviewed changes

schmidt-sebastian added 2 commits June 16, 2020 22:23

Review

c1744ca

Merge

cbdb633

schmidt-sebastian assigned wilhuff and unassigned Feiyang1 and schmidt-sebastian Jun 17, 2020

wilhuff reviewed Jun 17, 2020

View reviewed changes

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

Review

bbf2dec

schmidt-sebastian commented Jun 17, 2020

View reviewed changes

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

wilhuff reviewed Jun 19, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jun 19, 2020

schmidt-sebastian added 2 commits June 19, 2020 13:33

Review

5648cde

Merge

6174061

schmidt-sebastian requested review from avolkovi, sam-gc, scottcrossen and yuchenshi as code owners June 19, 2020 20:45

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Jun 19, 2020

yuchenshi approved these changes Jun 19, 2020

View reviewed changes

wilhuff approved these changes Jun 19, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jun 19, 2020

schmidt-sebastian merged commit 0b9ef3a into master Jun 19, 2020

schmidt-sebastian deleted the mrschmidt/treeshakeableplatform branch July 9, 2020 17:44

firebase locked and limited conversation to collaborators Jul 20, 2020

		@@ -0,0 +1,4 @@
		The modules in this directory provide implementations for platform-specific features that are replaced at build time

Replace Platform injection with source preprocessor #3206

Replace Platform injection with source preprocessor #3206

Uh oh!

Conversation

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

Uh oh!

Uh oh!

google-oss-bot commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/lite

@firebase/firestore/memory

firebase

Test Logs

Uh oh!

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

Uh oh!

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

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!

Feiyang1 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!

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

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!

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 Jun 11, 2020 •

edited

Loading

google-oss-bot commented Jun 11, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

schmidt-sebastian commented Jun 12, 2020 •

edited

Loading