-
Notifications
You must be signed in to change notification settings - Fork 946
Enable bundle reader for Node. #3167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
9602712
c5e783e
5e7fb89
1ee1615
18f0be1
aa455bf
78248cd
83160a1
24e10cb
9d6edc5
4cbe608
4313e51
de1d162
1775298
6eafb6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,11 @@ export interface Platform { | |
*/ | ||
randomBytes(nBytes: number): Uint8Array; | ||
|
||
/** | ||
* Builds a `ByteStreamReader` from a data source. | ||
*/ | ||
toByteStreamReader(source: unknown): ByteStreamReader; | ||
|
||
/** The Platform's 'window' implementation or null if not available. */ | ||
readonly window: Window | null; | ||
|
||
|
@@ -60,6 +65,54 @@ export interface Platform { | |
readonly base64Available: boolean; | ||
} | ||
|
||
/** | ||
* An interface compatible with Web's ReadableStream.getReader() return type. | ||
* | ||
* This can be used as an abstraction to mimic `ReadableStream` where it is not | ||
* available. | ||
*/ | ||
export interface ByteStreamReader { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you take a look at using the type
(from lib.dom.d.ts) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the document, it seems
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
As for 3: Right now, we ignore the reason for cancelling. Could we just make it part of the error message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I had the assumption that ReadableStreamReader will not be available for Node, turns out the interface does exist with |
||
read(): Promise<ByteStreamReadResult>; | ||
cancel(reason?: string): Promise<void>; | ||
} | ||
|
||
/** | ||
* An interface compatible with ReadableStreamReadResult<UInt8Array>. | ||
*/ | ||
export interface ByteStreamReadResult { | ||
done: boolean; | ||
value?: Uint8Array; | ||
} | ||
|
||
/** | ||
* Builds a `ByteStreamReader` from a UInt8Array. | ||
* @param source The data source to use. | ||
* @param bytesPerRead How many bytes each `read()` from the returned reader | ||
* will read. | ||
*/ | ||
export function toByteStreamReader( | ||
source: Uint8Array, | ||
bytesPerRead = 10240 | ||
): ByteStreamReader { | ||
let readFrom = 0; | ||
return new (class implements ByteStreamReader { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you are using the You could just use: const reader : ByteStreamReader = { There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
async read(): Promise<ByteStreamReadResult> { | ||
if (readFrom < source.byteLength) { | ||
const result = { | ||
value: source.slice(readFrom, readFrom + bytesPerRead), | ||
done: false | ||
}; | ||
readFrom += bytesPerRead; | ||
return result; | ||
} | ||
|
||
return { value: undefined, done: true }; | ||
} | ||
|
||
async cancel(reason?: string): Promise<void> {} | ||
})(); | ||
} | ||
|
||
/** | ||
* Provides singleton helpers where setup code can inject a platform at runtime. | ||
* setPlatform needs to be set before Firestore is used and must be set exactly | ||
|
@@ -74,6 +127,13 @@ export class PlatformSupport { | |
PlatformSupport.platform = platform; | ||
} | ||
|
||
/** | ||
* Forcing to set the platform instance, testing only! | ||
*/ | ||
private static _forceSetPlatform(platform: Platform): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can drop this now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
PlatformSupport.platform = platform; | ||
} | ||
|
||
static getPlatform(): Platform { | ||
if (!PlatformSupport.platform) { | ||
fail('Platform not set'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,12 @@ | |
*/ | ||
|
||
import { DatabaseId, DatabaseInfo } from '../core/database_info'; | ||
import { Platform } from '../platform/platform'; | ||
import { | ||
ByteStreamReader, | ||
ByteStreamReadResult, | ||
Platform, | ||
toByteStreamReader | ||
} from '../platform/platform'; | ||
import { Connection } from '../remote/connection'; | ||
import { JsonProtoSerializer } from '../remote/serializer'; | ||
import { ConnectivityMonitor } from './../remote/connectivity_monitor'; | ||
|
@@ -93,4 +98,33 @@ export class BrowserPlatform implements Platform { | |
} | ||
return bytes; | ||
} | ||
|
||
/** | ||
* On web, a `ReadableStream` is wrapped around by a `ByteStreamReader`. | ||
*/ | ||
toByteStreamReader( | ||
source: Uint8Array | ArrayBuffer | ReadableStream<Uint8Array> | ||
): ByteStreamReader { | ||
if (source instanceof Uint8Array) { | ||
return toByteStreamReader(source); | ||
} | ||
if (source instanceof ArrayBuffer) { | ||
return toByteStreamReader(new Uint8Array(source)); | ||
} | ||
if (source instanceof ReadableStream) { | ||
const reader = source.getReader(); | ||
return new (class implements ByteStreamReader { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To my surprise, yes! Thanks. |
||
read(): Promise<ByteStreamReadResult> { | ||
return reader.read(); | ||
} | ||
|
||
cancel(reason?: string): Promise<void> { | ||
return reader.cancel(reason); | ||
} | ||
})(); | ||
} | ||
throw new Error( | ||
'Source of `toByteStreamReader` has to be Uint8Array, ArrayBuffer or ReadableStream' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -19,7 +19,11 @@ import { randomBytes } from 'crypto'; | |||
import { inspect } from 'util'; | ||||
|
||||
import { DatabaseId, DatabaseInfo } from '../core/database_info'; | ||||
import { Platform } from '../platform/platform'; | ||||
import { | ||||
ByteStreamReader, | ||||
Platform, | ||||
toByteStreamReader | ||||
} from '../platform/platform'; | ||||
import { Connection } from '../remote/connection'; | ||||
import { JsonProtoSerializer } from '../remote/serializer'; | ||||
import { Code, FirestoreError } from '../util/error'; | ||||
|
@@ -83,4 +87,14 @@ export class NodePlatform implements Platform { | |||
|
||||
return randomBytes(nBytes); | ||||
} | ||||
|
||||
/** | ||||
* On Node, only supported data source is a `Uint8Array` for now. | ||||
*/ | ||||
toByteStreamReader(source: unknown): ByteStreamReader { | ||||
if (source instanceof Uint8Array) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest you use this pattern here:
If it doesn't apply here, then you should invoke it at the callsite (the API that the end user calls) and add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||
return toByteStreamReader(source); | ||||
} | ||||
throw new Error('Source of `toByteStreamReader` has to be Uint8Array'); | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,10 +16,34 @@ | |
*/ | ||
|
||
import { expect } from 'chai'; | ||
import { PlatformSupport } from '../../../src/platform/platform'; | ||
import { | ||
PlatformSupport, | ||
toByteStreamReader | ||
} from '../../../src/platform/platform'; | ||
|
||
describe('Platform', () => { | ||
it('can load the platform at runtime', () => { | ||
expect(PlatformSupport.getPlatform()).to.exist; | ||
}); | ||
|
||
it('toByteStreamReader() steps underlying data', async () => { | ||
const encoder = new TextEncoder(); | ||
const r = toByteStreamReader(encoder.encode('0123456789'), 4); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add inline comment to explain what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
let result = await r.read(); | ||
expect(result.value).to.deep.equal(encoder.encode('0123')); | ||
expect(result.done).to.be.false; | ||
|
||
result = await r.read(); | ||
expect(result.value).to.deep.equal(encoder.encode('4567')); | ||
expect(result.done).to.be.false; | ||
|
||
result = await r.read(); | ||
expect(result.value).to.deep.equal(encoder.encode('89')); | ||
expect(result.done).to.be.false; | ||
|
||
result = await r.read(); | ||
expect(result.value).to.be.undefined; | ||
expect(result.done).to.be.true; | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.