Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
experimentalForceOwningTab
for Web Worker support #2197New 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
Add
experimentalForceOwningTab
for Web Worker support #2197Changes from 31 commits
f0287d4
15cf32e
66655d0
9ac6e2d
d813905
347b722
91404e3
00c6dc7
6dc5e51
fef8971
8366646
741e88e
86ca4aa
16479d4
0784827
59d07ee
5dae5b6
71785f2
ca02909
c5f491a
08f8fb9
398c330
6b6db57
b178a75
7d8e41a
6d20b20
016aed8
31a8772
c7cbf9a
be3a0d4
47cd1f6
0f45287
9cfe34d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Was the intention to use backticks around
true
? That would work.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.
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.
note: If/when we ship this for real we should add more detail about exactly how other tabs will fail (so that developers can potentially write code to detect it, etc.).
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.
Just want to jot this down so I can point to it in 20 years: This name is so amazingly non-obvious that it really bothers me. It bothers me even more that there was no feedback on this from the API council. I don't understand how that this got approved without a single comment from Michael Bleigh, who has been the one true shepherd of the Firestore Web API. It is also still technically missing one more LGTM, as the Games TL is not yet a voting council member.
Since I don't want to just rant without providing an alternative suggestion, here you go:
experimentalForceExclusivePersistenceAccess
experimentalForcePersistenceOwnership
I also think that this could work with
synchronizeTabs
if it is used outside of WebWorkers. If this is not a pattern we want to support, then maybe we should have WebWorkers in the name?experimentalWebWorkerPersistence
We don't necessarily need to open this up for discussion again, but I can almost guarantee that if you randomly surveyed people outside of our team, no one would be able to say what
experimentalForceOwningTab
does and why they would need to turn it on to use Firestore with WebWorkers.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.
What is the use case for this feature with synchronizeTabs? Limiting the name to web workers only would be misleading, since this should work for Web, Shared, and Service Workers as well as other non-worker environments that don't have LocalStorage availability.
Not sure what proper next steps are here. I can definitely see your perspective on how
forceOwningTabs
would be confusing, but I'm not sure how to reduce confusion without introducing the concept of persistence leases, which was a decision factor during the API discussion. One option I see is to release this asexperimentalForceOwningTab
for now and then rename it to something else if/when we make remove the experimental flag on this based on developer feedback.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.
The use case might be to pin an owner tab. Not sure why that might be useful, but it would avoid primary to secondary tab transition which AFAIK haven't been a problem.
As for the name:
To me,
WebWorker
is the umbrella term, but I might be mistaken (and proposeexperimentalBackgroundWorkerPersistence
instead).My main problem with the API name is that
experimentalForceOwningTab
only says what it does but doesn't say why it does it or what feature it unblocks. The fact that it enables WebWorker/SharedWorker/ServiceWorker persistence is not obvious from the name, and it isn't even document in the API description. The only place where it shows up is in the title of the issue that is linked. A user that wants to use Firestore in a WebWorker has no obvious way to discover that this is the setting they need to enable.If you are happy with the name, feel free to ignore. This has been discussed for quite some time.
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.
Do you mind adding a short inline comment for this and
allowTabSynchronization
? The rest of the arguments seem obvious.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.
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.
Ideally, this would say something about what the consequences are:
"If set to true, forcefully obtains database access. Existing tabs will no longer be able to access IndexedDB."
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.
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.
Multi-Tab persistence requires LocalStore. I think WebSharedStorageClientState will reject startup without it, but it might be worth getting a second pair of eyes to look at this code and its callers:
firebase-js-sdk/packages/firestore/src/local/shared_client_state.ts
Line 503 in 6cfb268
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.
We now throw an error if
synchronizeTabs
is used withexperimentalForceOwningTab
, and we throw an error if LocalStorage is unavailable in WebStorageSharedClientState. Which case are we not covering?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.
We used to throw here as well if LocalStorage wasn't available. It looks like you came to the same conclusion as I did and that we are still throwing, just somewhere else. LGTM.
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.
You can probably drop the generic since it can be inferred.
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.
Not sure why TS can't infer it, but the error I get is:
Type 'PersistencePromise<true>' is not assignable to type 'PersistencePromise<boolean>'.
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.
I am surprised this works with the IndexedDB shim. We only register on
window.indexedDB
:firebase-js-sdk/packages/firestore/test/util/test_platform.ts
Line 57 in 631a0ec
The CI is green though.