Enable bundle reader for Node. #3167

wu-hui · 2020-06-05T02:17:54Z

No description provided.

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

google-oss-bot · 2020-06-05T15:09:29Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (b0c8299) Head (ab6d188) Diff

browser 252 kB 253 kB +915 B (+0.4%)

esm2017 195 kB 196 kB +538 B (+0.3%)

main 494 kB 495 kB +943 B (+0.2%)

module 250 kB 250 kB +871 B (+0.3%)
@firebase/firestore/memory

Type Base (b0c8299) Head (ab6d188) Diff

browser 192 kB 193 kB +915 B (+0.5%)

esm2017 149 kB 150 kB +538 B (+0.4%)

main 369 kB 370 kB +943 B (+0.3%)

module 190 kB 191 kB +871 B (+0.5%)

`firebase`

Type	Base (`b0c8299`)	Head (`ab6d188`)	Diff
firebase-firestore.js	290 kB	291 kB	+823 B (+0.3%)
firebase-firestore.memory.js	232 kB	232 kB	+823 B (+0.4%)
firebase.js	823 kB	824 kB	+827 B (+0.1%)

Test Logs

Base (b0c82994): https://github.com/firebase/firebase-js-sdk/actions/runs/125439589
Head (ab6d1881): https://github.com/firebase/firebase-js-sdk/actions/runs/126087221

schmidt-sebastian · 2020-06-06T22:18:50Z

packages/firestore/src/platform/platform.ts

+ * This can be used as an abstraction to mimic `ReadableStream` where it is not
+ * available.
+ */
+export interface ByteStreamReader {


Did you take a look at using the type ReadableStreamReader directly?

interface ReadableStreamReader<R = any> { cancel(): Promise<void>; read(): Promise<ReadableStreamReadResult<R>>; releaseLock(): void; }

(from lib.dom.d.ts)

Looking at the document, it seems ReadableStreamDefaultReader is the interface? There are several differences.

releaseLock() is not added there because it is not used. I dont have a strong preference though.

The DOM one is more generic, but given we only intend to use it to read bundle source which is ArrayBuffer | ReadableStream<UInt8Array> I think it is fine?

cancel takes a string because ReadableStreamDefaultReader takes a reason for its cancel (https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/cancel).

I think we would be better off if we re-used existing types. It makes our code smaller, easier to understand for outsiders and is more future proof. You are already using most of the idioms of the existing API, and re-using the type would be a good explanation of why you are doing this ("citing the source").

The change here is actually pretty simple: https://gist.github.com/schmidt-sebastian/62cabbbcab7f78f3a6e1aee7809f1b73
If we need a more complicated type, we could keep ByteStreamReader but make it:

export type ByteStreamReader = ReadableStreamReader<ArrayBuffer | ReadableStream<UInt8Array>>

As for 3: Right now, we ignore the reason for cancelling. Could we just make it part of the error message?

Done.

I had the assumption that ReadableStreamReader will not be available for Node, turns out the interface does exist with d.ts. Thanks for the patch!

schmidt-sebastian · 2020-06-06T22:24:54Z

packages/firestore/src/platform/platform.ts

+  bytesPerRead = 10240
+): ByteStreamReader {
+  let readFrom = 0;
+  return new (class implements ByteStreamReader {


I assume you are using the IIFE pattern here to enforce the ByteStreamReader type?

You could just use:

const reader : ByteStreamReader = {
...
}
return reader;

schmidt-sebastian · 2020-06-06T22:28:57Z

packages/firestore/src/platform_browser/browser_platform.ts

+    }
+    if (source instanceof ReadableStream) {
+      const reader = source.getReader();
+      return new (class implements ByteStreamReader {


Can we just return reader here?

To my surprise, yes! Thanks.

schmidt-sebastian · 2020-06-06T22:29:39Z

packages/firestore/src/platform_browser/browser_platform.ts

+      })();
+    }
+    throw new Error(
+      'Source of `toByteStreamReader` has to be Uint8Array, ArrayBuffer or ReadableStream'


Missing a ("has to be a Uint8Array")

schmidt-sebastian · 2020-06-06T22:30:34Z

packages/firestore/src/platform/platform.ts

+  /**
+   * Builds a `ByteStreamReader` from a data source.
+   */
+  toByteStreamReader(source: unknown): ByteStreamReader;


Can the source type be Uint8Array | ArrayBuffer | ReadableStream<Uint8Array>?

And since this type combination is used widely now, should we add a local type alias?

schmidt-sebastian · 2020-06-06T22:32:57Z

packages/firestore/src/platform_node/node_platform.ts

+   * On Node, only supported data source is a `Uint8Array` for now.
+   */
+  toByteStreamReader(source: unknown): ByteStreamReader {
+    if (source instanceof Uint8Array) {


I suggest you use this pattern here:

firebase-js-sdk/packages/firestore/src/api/blob.ts

Line 81 in e67affb

if (!(array instanceof Uint8Array)) {

If it doesn't apply here, then you should invoke it at the callsite (the API that the end user calls) and add a debugAssert here.

schmidt-sebastian · 2020-06-06T22:33:58Z

packages/firestore/test/unit/platform/platform.test.ts

+
+  it('toByteStreamReader() steps underlying data', async () => {
+    const encoder = new TextEncoder();
+    const r = toByteStreamReader(encoder.encode('0123456789'), 4);


Add inline comment to explain what 4 is.

schmidt-sebastian · 2020-06-06T22:35:44Z

packages/firestore/test/unit/util/bundle.test.ts

-): ReadableStream {
-  return toReadableStream(new TextEncoder().encode(content), bytesPerRead);
+  bytesPerRead = 10240
+): ReadableStream<Uint8Array | ArrayBuffer> {


Would it be possible to convert the input to a UInt8Array and re-use toByteStreamReader?

Done with some tweak.

schmidt-sebastian · 2020-06-06T22:37:30Z

packages/firestore/test/util/test_platform.ts

@@ -270,6 +270,10 @@ export class TestPlatform implements Platform {
  randomBytes(nBytes: number): Uint8Array {
    return this.basePlatform.randomBytes(nBytes);
  }
+
+  toByteStreamReader(source: unknown): ByteStreamReader {
+    return this.basePlatform.toByteStreamReader(source);


I suspect that if you move your Node-specifc genericBundleReadingTests logic here, then you can use the TestPlatform throughout and won't have to force override it.

See above. It now works for both platforms without hacking.

schmidt-sebastian · 2020-06-06T22:40:41Z

packages/firestore/test/unit/util/bundle.test.ts

+    platform.toByteStreamReader = source =>
+      toByteStreamReader(source as Uint8Array, bytesPerRead);
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    (PlatformSupport as any)._forceSetPlatform(platform);


I think you should try really hard to to have to do this. Options:

Have a Node specific test file (bundle.node.test.ts similar to serializer.node.test.ts)

Implement this logic in TestPlatform's toByteStreamReader.

A very strong argument against this pattern is that guaranteeing cleanup for this type of global state modification is hard. It even looks like you opted not do to it at all :)

Actually making toByteStreamReader take a bytesPerRead eliminates the problem.

wu-hui

Thanks for the review!

wu-hui · 2020-06-09T20:01:26Z

packages/firestore/src/platform/platform.ts

+  /**
+   * Builds a `ByteStreamReader` from a data source.
+   */
+  toByteStreamReader(source: unknown): ByteStreamReader;


wu-hui · 2020-06-09T20:15:10Z

packages/firestore/src/platform/platform.ts

+ * This can be used as an abstraction to mimic `ReadableStream` where it is not
+ * available.
+ */
+export interface ByteStreamReader {


Looking at the document, it seems ReadableStreamDefaultReader is the interface? There are several differences.

releaseLock() is not added there because it is not used. I dont have a strong preference though.

The DOM one is more generic, but given we only intend to use it to read bundle source which is ArrayBuffer | ReadableStream<UInt8Array> I think it is fine?

cancel takes a string because ReadableStreamDefaultReader takes a reason for its cancel (https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/cancel).

wu-hui · 2020-06-09T20:30:49Z

packages/firestore/src/platform/platform.ts

+  bytesPerRead = 10240
+): ByteStreamReader {
+  let readFrom = 0;
+  return new (class implements ByteStreamReader {


wu-hui · 2020-06-09T20:34:24Z

packages/firestore/src/platform_browser/browser_platform.ts

+    }
+    if (source instanceof ReadableStream) {
+      const reader = source.getReader();
+      return new (class implements ByteStreamReader {


To my surprise, yes! Thanks.

wu-hui · 2020-06-09T20:34:35Z

packages/firestore/src/platform_browser/browser_platform.ts

+      })();
+    }
+    throw new Error(
+      'Source of `toByteStreamReader` has to be Uint8Array, ArrayBuffer or ReadableStream'


wu-hui · 2020-06-09T21:00:55Z

packages/firestore/test/unit/util/bundle.test.ts

-): ReadableStream {
-  return toReadableStream(new TextEncoder().encode(content), bytesPerRead);
+  bytesPerRead = 10240
+): ReadableStream<Uint8Array | ArrayBuffer> {


Done with some tweak.

wu-hui · 2020-06-10T01:00:26Z

packages/firestore/src/platform_node/node_platform.ts

+   * On Node, only supported data source is a `Uint8Array` for now.
+   */
+  toByteStreamReader(source: unknown): ByteStreamReader {
+    if (source instanceof Uint8Array) {


wu-hui · 2020-06-10T01:00:42Z

packages/firestore/test/unit/platform/platform.test.ts

+
+  it('toByteStreamReader() steps underlying data', async () => {
+    const encoder = new TextEncoder();
+    const r = toByteStreamReader(encoder.encode('0123456789'), 4);


wu-hui · 2020-06-10T01:01:37Z

packages/firestore/test/unit/util/bundle.test.ts

+    platform.toByteStreamReader = source =>
+      toByteStreamReader(source as Uint8Array, bytesPerRead);
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    (PlatformSupport as any)._forceSetPlatform(platform);


Actually making toByteStreamReader take a bytesPerRead eliminates the problem.

wu-hui · 2020-06-10T01:02:13Z

packages/firestore/test/util/test_platform.ts

@@ -270,6 +270,10 @@ export class TestPlatform implements Platform {
  randomBytes(nBytes: number): Uint8Array {
    return this.basePlatform.randomBytes(nBytes);
  }
+
+  toByteStreamReader(source: unknown): ByteStreamReader {
+    return this.basePlatform.toByteStreamReader(source);


See above. It now works for both platforms without hacking.

schmidt-sebastian · 2020-06-10T15:04:33Z

packages/firestore/src/platform/platform.ts

+  /**
+   * Forcing to set the platform instance, testing only!
+   */
+  private static _forceSetPlatform(platform: Platform): void {


You can drop this now.

schmidt-sebastian · 2020-06-10T15:05:59Z

packages/firestore/src/util/bundle_reader.ts

- * @param data: Underlying buffer.
- * @param bytesPerRead: How many bytes to read from the underlying buffer from
- * each read through the stream.
+ * It is not application when we don't really have control, for example, when


s/application/applicable

But it could just be: "Not applicable for ReadableStreams."

schmidt-sebastian · 2020-06-10T15:18:53Z

packages/firestore/src/platform/platform.ts

+ * This can be used as an abstraction to mimic `ReadableStream` where it is not
+ * available.
+ */
+export interface ByteStreamReader {


I think we would be better off if we re-used existing types. It makes our code smaller, easier to understand for outsiders and is more future proof. You are already using most of the idioms of the existing API, and re-using the type would be a good explanation of why you are doing this ("citing the source").

The change here is actually pretty simple: https://gist.github.com/schmidt-sebastian/62cabbbcab7f78f3a6e1aee7809f1b73
If we need a more complicated type, we could keep ByteStreamReader but make it:

export type ByteStreamReader = ReadableStreamReader<ArrayBuffer | ReadableStream<UInt8Array>>

As for 3: Right now, we ignore the reason for cancelling. Could we just make it part of the error message?

schmidt-sebastian · 2020-06-10T16:59:06Z

packages/firestore/src/platform/platform.ts

+  source: Uint8Array,
+  bytesPerRead: number
+): ReadableStreamReader<Uint8Array> {
+  validatePositiveNumber('toByteStreamReader', 2, bytesPerRead);


Since this is not a user facing method, this should be a debugAssert, which will get removed in the final build and drops the dependency on validatePositiveNumber in code that doesn't otherwise depend on it.

wu-hui added 12 commits May 21, 2020 13:49

Renaming interfaces without leading I

9602712

Initial commit of bundle reading - for web only.

c5e783e

Tests only run when it is not Node.

5e7fb89

Fix redundant imports

1ee1615

Fix missing textencoder

18f0be1

Remove generator.

aa455bf

Support bundle reader for Node

78248cd

Fix rebase errors.

83160a1

Remote 'only'

24e10cb

Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode

9d6edc5

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode

4cbe608

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Added more comments, and more tests for Node.

4313e51

wu-hui requested a review from schmidt-sebastian June 5, 2020 14:59

wu-hui assigned schmidt-sebastian Jun 5, 2020

wu-hui added the api: firestore label Jun 5, 2020

schmidt-sebastian reviewed Jun 6, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 6, 2020

Address comments and remove hacking.

de1d162

wu-hui commented Jun 10, 2020

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jun 10, 2020

wu-hui changed the title ~~Enable bundler reader for Node.~~ Enable bundle reader for Node. Jun 10, 2020

schmidt-sebastian reviewed Jun 10, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 10, 2020

Drop custom reader type.

1775298

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jun 10, 2020

schmidt-sebastian reviewed Jun 10, 2020

View reviewed changes

schmidt-sebastian approved these changes Jun 10, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Jun 10, 2020

Remove validation and use assertion.

6eafb6f

wu-hui merged commit 2f179bc into wuandy/Bundles Jun 10, 2020

firebase locked and limited conversation to collaborators Jul 11, 2020

Enable bundle reader for Node. #3167

Enable bundle reader for Node. #3167

Uh oh!

Conversation

wu-hui commented Jun 5, 2020

Uh oh!

google-oss-bot commented Jun 5, 2020

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/memory

firebase

Test Logs

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!

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!

wu-hui 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

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`