Tree-Shakeable Mutations #3329
Tree-Shakeable Mutations #3329
Conversation
🦋 Changeset is good to goLatest commit: 6c28d0b We got this. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
force-pushed
the
mrschmidt/mutations2
branch
from
285da41 to
9743538
Compare
schmidt-sebastian
requested review from
alikn,
andirayo,
avolkovi,
egilmorez,
erikeldridge,
sam-gc,
scottcrossen,
yuchenshi,
zijianjoy and
zwu52
as code owners
schmidt-sebastian
changed the title
| @@ -123,7 +123,7 @@ export class ServerTimestampFieldValueImpl extends SerializableFieldValue { | |||
| } | |||
|
|
|||
| _toFieldTransform(context: ParseContext): FieldTransform { | |||
| return new FieldTransform(context.path!, ServerTimestampTransform.instance); | |||
There was a problem hiding this comment.
Removed static instance member as these are not tree-shakeable. See #3264
There was a problem hiding this comment.
I noticed we reference all the transforms inside of transform_operation.ts. Will the branches in that file get erased if we don't reference ServerTimestampTransform? It seems like we have a problem either way. Am I missing something?
There was a problem hiding this comment.
Given the way our serializer works (we look at the data in IndexedDB and then decide which transform to create), I don't think it is possible to structure our SDK in a way that ServerTimestampTransform is not included if another transform is used. Having ServerTimestampTransform.instance however means that ServerTimestampTransform may also be included in the final bundle if neither the serializer not the transforms are used at all.
There was a problem hiding this comment.
That's what I figured. imho - I think that if the final bundle takes the hit of including these types regardless of use we should consider maintaining the existing pattern. I was never a fan of the new style and and if we're not getting any benefit from it, why deviate?
There was a problem hiding this comment.
The Lite SDK doesn't reference these methods and is able to remove them (see the binary size report: -7%). Does that answer your concern?
| if (precondition.updateTime !== undefined) { | ||
| return ( | ||
| maybeDoc instanceof Document && | ||
| maybeDoc.version.isEqual(precondition.updateTime) |
There was a problem hiding this comment.
Moved this method out of Precondition because it pulls in Document.
| * UnknownDocument if the mutation could not be applied to the locally | ||
| * cached base document. | ||
| */ | ||
| export function applyMutationToRemoteDocument( |
There was a problem hiding this comment.
Apologies in advance for this drive by comment :-).
Why is this pattern better than just having the lite SDK create the Write protos directly? This seems like way more code added and in the end the lite SDK still ends up with an intermediate object that contributes to the heft of the SDK.
When simplifying documents in the other SDKs we made them just wrap the protos. It seems like we could take a similar approach here: have the lite SDK to just build the protos and pass them down. The heavier-weight SDKs could reuse that code but then wrap the protos in classes that add local behavior.
This way we'd still end up with code sharing between the SDKs, the lite SDK ends up optimally lite (with every API call is just translation of arguments into protos and a REST invocation), but we don't end up having to deal with these instanceof ladders for behavior that doesn't fit in the lite SDK.
Note: this comment is really a question. I don't really understand why you disliked the outcome of #3327 and I don't understand what value mutations have in the lite SDK. If you're convinced this path is the way forward I'll be happy with that outcome but I just want to make sure because this pattern is kinda gross.
There was a problem hiding this comment.
The reason I don't like #3327:
- It duplicates the Proto encoding for users that use the Write-proto based Transactions and non-transactional writes. This leads to a general size increase for the main SDK and firestore-exp.
- It makes it impossible to share the WriteBatch between the firestore-exp and the Lite SDK.
- It adds a lot of functions with names that I am not happy with.
| @@ -123,7 +123,7 @@ export class ServerTimestampFieldValueImpl extends SerializableFieldValue { | |||
| } | |||
|
|
|||
| _toFieldTransform(context: ParseContext): FieldTransform { | |||
| return new FieldTransform(context.path!, ServerTimestampTransform.instance); | |||
There was a problem hiding this comment.
I noticed we reference all the transforms inside of transform_operation.ts. Will the branches in that file get erased if we don't reference ServerTimestampTransform? It seems like we have a problem either way. Am I missing something?
The goal of this PR is to remove the latency-compensation only features that Mutations currently offer. The idea is to include
applyToLocalViewandapplyToRemoteDocumentonly in the full-featured SDK. While the approach in this SDK ties to different Mutation types together (a build cannot just depend onPatchMutation), this is already the case and I think it will be close to impossible to make the Mutations independent, as the are always tied together in the serializer.Note that I tried a different approach: #3327 makes it so the Lite doesn't use Mutations at all, instead it uses writes. I am not a fan of how that turned out though.
Diff without whitespace: #3329 (much smaller)