Skip to content
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

fix: block change serialization #7400

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

BeksOmega
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes N/A

Proposed Changes

Switches change events (i.e. for mutations) to use full serialization.

Reason for Changes

I am reasonably sure this is the behavior it should have. But I needed to do a lot of playing with the shared procedure blocks to be confident.

Events are built for two use cases:

  1. Notifying the developer when things happen.
  2. Mirroring workspaces.

If we don't do full serialization then (1) breaks. E.g. in the shared procedure blocks change events will not be fired when the blocks mutate because their extra state isn't changing (their just serializing the same procedure ID). (2) is still fine because mirroring the procedure model updates updates the blocks.

With full serialization, (2) is a bit tricky because you need to mirror the changes to blocks and makes sure /not/ to mirror the changes to data models. If you mirror both, the block will be mirrored and create a new data model, and then the original data model will be mirrored (so you end up with an extra data model).

Note that we can't have the block create a data model with the same ID because we don't want copy-pasted blocks to point at the same procedure model as their originator.

Also note that the trickiness /currently/ applies to the released code where doFullSerialization doesn't exist. This change doesn't make mirroring better or worse than the released code. But it does make it trickier than the unreleased code :P

Test Coverage

N/A

Documentation

N/A

Additional Information

N/A

@BeksOmega BeksOmega requested a review from a team as a code owner August 16, 2023 21:40
@github-actions github-actions bot added the PR: fix Fixes a bug label Aug 16, 2023
@BeksOmega BeksOmega merged commit 9efd944 into google:develop Aug 16, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants