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: make deserialization use the new render management system #7306

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Jul 19, 2023

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 #7290

Proposed Changes

Changed both the JSON and XML deserialization systems to use the render queue, but trigger an immediate render.

Reason for Changes

Moving both sytems to use the render queue allows us to get rid of the old render management system. But I chose to make them trigger an immediate render (instead of waiting until the end of the frame) to ensure 100% backwards compatibility.

We could have people use the finishQueuedRenders method if they want to calculate things based on the size/position of the blocks after deserialization. However, as we discovered last time I was reorganizing stuff, that's technically a breaking change :/ And I think deserialization is a much more high-risk area for this kind of thing! (as opposed to like, adding an input to a block).

Test Coverage

  • Verified that there is no performance impact for deserialization. The spaghetti tests ran in the same amount of time before and after this change.
  • Verified that copying and pasting properly renders the block.
  • Verified that setting shadows programatically properly renders the block.

Documentation

N/A

Additional Information

N/A

Dependent on: #7296

@BeksOmega BeksOmega requested a review from a team as a code owner July 19, 2023 16:22
@BeksOmega BeksOmega requested a review from maribethb July 19, 2023 16:22
@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 19, 2023
@BeksOmega BeksOmega marked this pull request as draft July 19, 2023 17:48
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 19, 2023
@BeksOmega BeksOmega mentioned this pull request Jul 19, 2023
4 tasks
core/flyout_base.ts Show resolved Hide resolved
core/icons/comment_icon.ts Outdated Show resolved Hide resolved
* @returns The root block created.
* @internal
*/
export function domToBlockInternal(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get why you made domToBlock render immediately, but I don't love this name. It's not very descriptive, and it implies external developers would never want to call it. But in reality they might be perfectly fine not triggering an immediate render. What about domToBlockAsync? or something better than that in the same vein? Then we don't have to mark it as internal.

Or is your plan to eventually make the breaking change to domToBlock? in that case i don't care quite as much if this method is intended to be temporary

Copy link
Collaborator Author

@BeksOmega BeksOmega Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My plan is indeed to eventually make the breaking change to domToBlock (unless there's an important reason we don't want to do that)! I chose the domToBlockInternal name to match the naming of the JSON serialization system.

@maribethb
Copy link
Contributor

sorry didn't realize this was a draft

@BeksOmega BeksOmega marked this pull request as ready for review July 24, 2023 21:57
@BeksOmega BeksOmega merged commit 435e854 into google:develop Jul 26, 2023
@BeksOmega BeksOmega deleted the fix/queue-deserialization branch May 14, 2024 16:28
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.

Make JSON deserialization use the render queue
2 participants