-
Notifications
You must be signed in to change notification settings - Fork 944
Bundles load from secondary tabs #3393
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 32 commits
9602712
c5e783e
5e7fb89
1ee1615
18f0be1
aa455bf
78248cd
83160a1
24e10cb
9d6edc5
4cbe608
4313e51
296cfc4
fff3d36
fb762de
1ec4182
cd3ab7a
d991c75
af097c5
e735e23
17ba434
f808d8d
db1d864
979ffd9
f7ff495
556a007
adf1504
b364ab0
2588f20
93bc964
99610d7
e18493a
d175a14
5a4d0f2
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 |
---|---|---|
|
@@ -115,6 +115,18 @@ export function createWebStorageOnlineStateKey(persistenceKey: string): string { | |
return `${ONLINE_STATE_KEY_PREFIX}_${persistenceKey}`; | ||
} | ||
|
||
// The WebStorage prefix that plays as a event to indicate the remote documents | ||
// might have changed due to some secondary tabs loading a bundle. | ||
// format of the key is: | ||
// firestore_remote_documents_changed_<persistenceKey> | ||
export const REMOTE_DOCUMENTS_LOAD_FROM_BUNDLE_KEY_PREFIX = | ||
'firestore_remote_documents_changed'; | ||
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. So the remote documents change when queries get updated and even when mutations are committed, but this even only fires when a bundle is loaded. Should this be a bundle specific notification? 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. Please also update the key prefix. For sake of brevity, you could consider dropping "remote documents" altogether and just make it something like "bundle loaded". 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. |
||
export function createRemoteDocumentsLoadFromBundleKey( | ||
persistenceKey: string | ||
): string { | ||
return `${REMOTE_DOCUMENTS_LOAD_FROM_BUNDLE_KEY_PREFIX}_${persistenceKey}`; | ||
} | ||
|
||
/** | ||
* The JSON representation of the system's online state, as written by the | ||
* primary client. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,3 +28,17 @@ export function getDocument(): Document | null { | |
// eslint-disable-next-line no-restricted-globals | ||
return typeof document !== 'undefined' ? document : null; | ||
} | ||
|
||
/** | ||
* An instance of the Platform's 'TextEncoder' implementation. | ||
*/ | ||
export function newTextEncoder(): TextEncoder { | ||
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. This should only be in platform is there is some divergence between platforms. This doesn't seem to be the case? 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.
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. Sad :/ Should we move this to the ./serializer.ts file though? It seems much more fitting. 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. Hmm, but it is not used in 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 mean to move it from packages/firestore/src/platform/browser/dom.ts to packages/firestore/src/platform/browser/serializer.ts. It has little to do with DOM, mostly because it exists in Node which doesn't have DOM. 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. Ah, sorry. Done. |
||
return new TextEncoder(); | ||
} | ||
|
||
/** | ||
* An instance of the Platform's 'TextDecoder' implementation. | ||
*/ | ||
export function newTextDecoder(): TextDecoder { | ||
return new TextDecoder('utf-8'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import { | |
import { Deferred } from './promise'; | ||
import { debugAssert } from './assert'; | ||
import { toByteStreamReader } from '../platform/byte_stream_reader'; | ||
import { newTextDecoder } from '../platform/dom'; | ||
|
||
/** | ||
* A complete element in the bundle stream, together with the byte length it | ||
|
@@ -67,7 +68,7 @@ export class BundleReader { | |
*/ | ||
private buffer: Uint8Array = new Uint8Array(); | ||
/** The decoder used to parse binary data into strings. */ | ||
private textDecoder = new TextDecoder('utf-8'); | ||
private textDecoder: TextDecoder | null; | ||
|
||
static fromBundleSource(source: BundleSource): BundleReader { | ||
return new BundleReader(toByteStreamReader(source, BYTES_PER_READ)); | ||
|
@@ -77,6 +78,8 @@ export class BundleReader { | |
/** The reader to read from underlying binary bundle data source. */ | ||
private reader: ReadableStreamReader<Uint8Array> | ||
) { | ||
this.textDecoder = newTextDecoder(); | ||
debugAssert(!!this.textDecoder, 'Cannot create a valid text decoder.'); | ||
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. This would never fire, would it? 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. Right, fixed. |
||
// Read the metadata (which is the first element). | ||
this.nextElementImpl().then( | ||
element => { | ||
|
@@ -131,7 +134,7 @@ export class BundleReader { | |
return null; | ||
} | ||
|
||
const lengthString = this.textDecoder.decode(lengthBuffer); | ||
const lengthString = this.textDecoder!.decode(lengthBuffer); | ||
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. Drop bang. 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. |
||
const length = Number(lengthString); | ||
if (isNaN(length)) { | ||
this.raiseError(`length string (${lengthString}) is not valid number`); | ||
|
@@ -199,7 +202,7 @@ export class BundleReader { | |
} | ||
} | ||
|
||
const result = this.textDecoder.decode(this.buffer.slice(0, length)); | ||
const result = this.textDecoder!.decode(this.buffer.slice(0, length)); | ||
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. Drop bang. 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. |
||
// Update the internal buffer to drop the read json string. | ||
this.buffer = this.buffer.slice(length); | ||
return result; | ||
|
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.
createRemoteDocumentsLoadFromBundleKey
seems like it needs a name update now as well.createBundleLoadedKey
?For sake of brevity, I would also change bundleChangedRemoteDocumentsKey to bundleLoadedKey. I seems shorter and more specific.
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.