-
Notifications
You must be signed in to change notification settings - Fork 569
Improve applySerializedChange to support cross-session usage and transactions
#26662
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
86346aa
8ebc2d2
bd6249e
b82ee62
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 |
|---|---|---|
|
|
@@ -715,6 +715,7 @@ export class TreeCheckout implements ITreeCheckoutFork { | |
| idCompressor: this.idCompressor, | ||
| originatorId: this.idCompressor.localSessionId, | ||
| revision, | ||
| schema: undefined, // By not passing the schema, we avoid compressing identifiers in identifier fields | ||
noencke marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }; | ||
| const encodedChange = this.changeFamily.codecs.resolve(4).encode(change, context); | ||
|
|
||
|
|
@@ -775,19 +776,24 @@ export class TreeCheckout implements ITreeCheckoutFork { | |
| throw new UsageError(`Cannot apply change. Invalid serialized change format.`); | ||
| } | ||
| const { revision, originatorId, change } = serializedChange; | ||
| if (originatorId !== this.idCompressor.localSessionId) { | ||
| throw new UsageError( | ||
| `Cannot apply change. A serialized changed must be applied to the same SharedTree as it was created from.`, | ||
| ); | ||
| } | ||
| const context: ChangeEncodingContext = { | ||
| idCompressor: this.idCompressor, | ||
| originatorId: this.idCompressor.localSessionId, | ||
| originatorId, | ||
| revision, | ||
|
Contributor
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 probably all works, but it's kludgy that we use revision without decoding it. |
||
| }; | ||
| const decodedChange = this.changeFamily.codecs.resolve(4).decode(change, context); | ||
| // Apply the change to the branch, but _not_ the `activeBranch` - we do not support squashing serialized commits in a transaction. | ||
| this.#transaction.branch.apply(tagChange(decodedChange, revision)); | ||
|
|
||
| // Extract the inner data change from the SharedTreeChange envelope. | ||
| // Serialized changes are always single data changes (not schema changes). | ||
| const innerChange = decodedChange.changes[0]; | ||
| assert( | ||
| decodedChange.changes.length === 1 && innerChange?.type === "data", | ||
| 0x1b2 /* Expected a single data change in serialized change */, | ||
| ); | ||
|
|
||
| // Delegate to the editor, which will replace the revision, shift local IDs to avoid | ||
| // collisions with other changes in the same transaction, and apply the change. | ||
| this.#transaction.activeBranchEditor.applyExternalChange(innerChange.innerChange); | ||
| } | ||
|
|
||
| // Revision is the revision of the commit, if any, which caused this change. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -332,12 +332,11 @@ export interface TreeBranchAlpha extends TreeBranch, TreeContextAlpha { | |||||||||
| * Apply a serialized change to this branch. | ||||||||||
| * @param change - the change to apply. | ||||||||||
| * Changes are acquired via `getChange` in a branch's {@link TreeBranchEvents.changed | "changed"} event. | ||||||||||
| * @remarks Changes may only be applied to a SharedTree with the same IdCompressor instance and branch state from which they were generated. | ||||||||||
| * They may be created by one branch and applied to another, but only if both branches share the same history at the time of creation and application. | ||||||||||
| * @remarks Changes may only be applied to a SharedTree with the same branch state from which they were generated. | ||||||||||
| * They may be created by one branch and applied to another, but only if both branches share the same history at the time of creation and application, respectively. | ||||||||||
|
Comment on lines
+335
to
+336
|
||||||||||
| * @remarks Changes may only be applied to a SharedTree with the same branch state from which they were generated. | |
| * They may be created by one branch and applied to another, but only if both branches share the same history at the time of creation and application, respectively. | |
| * @remarks Changes may only be applied to a branch whose content/branch state (and compatible schema) matches the state from which they were generated. | |
| * They may be created by one branch and applied to another, including independently created views, as long as both branches are in an equivalent logical state at the time of creation and application (for example, by having applied the same sequence of edits or otherwise converged to the same content). |
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.
All the other editing methods take in a revision tag instead of minting one. Seems like we should be consistent here. Is it inconvenient to mint a revision at the call site?