Compat class for DocumentReference #4043
Conversation
schmidt-sebastian
requested review from
avolkovi,
jsdt and
sam-gc
as code owners
🦋 Changeset detectedLatest commit: 0862fdf The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check ✅
|
Binary Size ReportAffected SDKs
Test Logs |
…rebase-js-sdk into mrschmidt/documentref-compat
| @@ -37,7 +37,10 @@ registerAuth(ClientPlatform.WORKER); | |||
| export function getAuth(app = getApp()): Auth { | |||
| // Unlike the other environments, we need to explicitly check if indexedDb is | |||
| // available. That means doing the whole rigamarole | |||
| const auth = _getProvider(app, _ComponentName.AUTH).getImmediate() as AuthImpl; | |||
| const auth = _getProvider( | |||
There was a problem hiding this comment.
Are these changes to auth and database intended?
There was a problem hiding this comment.
No - they keep showing up though due to the Presubmit hook. I will revert them before merging.
| import { makeDatabaseInfo } from '../../lite/src/api/database'; | ||
| import { DEFAULT_HOST } from '../../lite/src/api/components'; | ||
| import * as exp from '../../exp/index'; |
There was a problem hiding this comment.
I thought * imports were to be avoided?
There was a problem hiding this comment.
Yeah, I should have caught this. Removed this import and aliased DocumentReference to ExpDocumentReference.
| internalOptions, | ||
| observer | ||
| const options = extractSnapshotOptions(args); | ||
| const observer = wrapObserver<DocumentSnapshot<T>, exp.DocumentSnapshot<T>>( |
There was a problem hiding this comment.
Above you're using import renaming to use ExpDocumentReference but here you're using a star import to get exp.DocumentSnapshot. We should do this consistently.
There was a problem hiding this comment.
Used ExpDocumentReference
| * Replaces the function name in an error thrown by the firestore-exp API | ||
| * with the function names used in the classic API. | ||
| */ | ||
| function rewriteError( |
There was a problem hiding this comment.
Since this is specific to method references maybe call this rewriteErrorMethod or maybe even just replaceMethod? Alternatively, leave it as a general rewriteError and make the caller supply the parens?
There was a problem hiding this comment.
Renamed to replaceFunctionName.
|
|
||
| /** | ||
| * Creates an observer that can be passed to the firestore-exp SDK. The | ||
| * observer converts all observed values into the format expected by the shim. |
There was a problem hiding this comment.
This is no longer a "shim", right?
This adds the Compat class for DocumentReference, along with the nasty hack "UntypedDataConverter" that treats the different FirestoreUserConverter (for all three SDKs) the same by ignoring the type of QuerySnapshot.
Note that this PR pulls in a lot of new code - I hope that I can get rid of most of this when I we add the QuerySnapshot Compat class.