Move handle transformation to SharedObject#18335
Move handle transformation to SharedObject#18335anthony-murphy wants to merge 5 commits intomicrosoft:mainfrom
Conversation
github-actions
bot
added
area: dds
…to mv-handle-transformtion
⯆ @fluid-example/bundle-size-tests: -12 Bytes
Baseline commit: bbc50a7 |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| this.services!.deltaConnection.submit(content, localOpMetadata); | ||
| this.services!.deltaConnection.submit( | ||
| makeHandlesSerializable(content, this.serializer, this.handle), |
There was a problem hiding this comment.
Note: reSubmitCore below will need to be updated as well, either to inline submitLocalMessage without this makeHandlesSerializable call, or to decode the handles first (but that seems like wasted CPU to round-trip; not worth it merely to share 2 straightforward lines of code)
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| this.services!.deltaConnection.submit(content, localOpMetadata); | ||
| this.services!.deltaConnection.submit( | ||
| makeHandlesSerializable(content, this.serializer, this.handle), |
There was a problem hiding this comment.
I'm curious what your approach would be for migrating external DDSes over. This appears to be a breaking behavior change to submitLocalMessage, in which case they'd need to update in the same PR that bumps to this version.
Although as I write that, I wonder if it's not actually, since I believe makeHandlesSerializable is idempotent.
There was a problem hiding this comment.
makeHandlesSerializable is idempotent, but we won't see those handles, so we'll want them to stop pre-encoding, probably want to log errors somehow
| localOpMetadata: unknown, | ||
| ) => { | ||
| this.process(message, local, localOpMetadata); | ||
| this.process(parseHandles(message, this.serializer), local, localOpMetadata); |
There was a problem hiding this comment.
I'm not sure why this would change. The end-to-end outbound path has the same result, and the inbound path wouldn't change. We could change it here, but might be better to keep for a follow-up. In the PR as it is, I feel like processCore in each DDS would need to be updated correspondingly to this change.
There was a problem hiding this comment.
since it is idempotent this doesn't hurt, and it would be best if dds didn't need to worry about it at all, its weird to say they need to decode but shouldn't encode
| }, | ||
| applyStashedOp: (content: any): unknown => { | ||
| return this.applyStashedOp(content); | ||
| return this.applyStashedOp(parseHandles(content, this.serializer)); |
There was a problem hiding this comment.
Same as above - I don't think this belongs in this PR.
There was a problem hiding this comment.
the goal is the op comes back is the same way it left. since it is idempotent this doesn't hurt, and it would be best if dds didn't need to worry about it at all, its weird to say they need to decode but shouldn't encode
3 participants
Move handle transformation to SharedObject