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/use editor state inside canvas #6099

Merged
merged 11 commits into from
Jul 19, 2024

Conversation

balazsbajorics
Copy link
Contributor

@balazsbajorics balazsbajorics commented Jul 18, 2024

Problem:
When anything in the projectContents changes, the DerivedState.remixData gets recreated. (This should be improved to only recreate the remixData if we actually change the file-based routing)

The UtopiaRemixRootComponent uses illegal (🙂) useEditorState hooks to access DerivedState.remixData.

Since DerivedState.remixData is referentially new, the useEditorState hooks will trigger a re-render of every UtopiaRemixRootComponent.

The UtopiaRemixRootComponent rerenders then basically trigger an almost full rerender of the canvas before the "real" canvas rerender step.

on master:
image
with Sean's createExecutionScope cache fix (currently disabled and blocked by #6091):
image

On the image you can see that in the sample store's / route this takes 15ms every single frame.

Fix:
20ms -> 10ms -> 5ms
image
When the canvas is in selective rerender mode, DO NOT let UtopiaRemixRootComponent rerender itself. Not updating UtopiaRemixRootComponent during the selective mode should be safe, since the continuous canvas interactions never cause the file based routing or loaders etc to update.

Commit Details: (< vv pls delete this section if's not relevant)

  • Wrapping the canvas in a EditorStateContext provider with the value forced to null which means we will throw an error in the future every time new code accidentally uses the useEditorState hooks (which are not compatible with selective re-rendering). Hopefully this means selective re-rendering will be less brittle in the future, but in a follow up PR I will write tests for this too
  • Added new hook useCanvasState which is almost the same as useEditorState except it takes an extra callback allowRerender. If AllowRerender is false, then no matter what changes in the editor state, we will passively reject the component update and the hook will return the old state value.
  • Using the new hook in utopia-remix-root-component
  • Profit!

Notes:
This PR depends on #6091 because the selective re-rendering of the remix root component causes the same css issue as Sean's original #6076

Manual Tests:
I hereby swear that:

  • I opened a hydrogen project and it loaded
  • I could navigate to various routes in Preview mode

Copy link
Contributor

github-actions bot commented Jul 18, 2024

Try me

Copy link

relativeci bot commented Jul 18, 2024

#13441 Bundle Size — 62.64MiB (~+0.01%).

28661d0(current) vs 6339ece master#13440(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 4 changes Regression 2 regressions
                 Current
#13441
     Baseline
#13440
Regression  Initial JS 45.71MiB(~+0.01%) 45.7MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 21.54% 21.52%
No change  Chunks 31 31
No change  Assets 34 34
Change  Modules 4376(+0.05%) 4374
Regression  Duplicate Modules 524(+0.19%) 523
No change  Duplicate Code 31.69% 31.69%
No change  Packages 469 469
No change  Duplicate Packages 70 70
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#13441
     Baseline
#13440
Regression  JS 62.62MiB (~+0.01%) 62.62MiB
Improvement  HTML 11.16KiB (-0.33%) 11.2KiB

Bundle analysis reportBranch fix/use-editor-state-inside-canv...Project dashboard

</CanvasContainer>
</UtopiaProjectCtxAtom.Provider>
</RerenderUtopiaCtxAtom.Provider>
{/* deliberately breaking useEditorState and useRefEditorState to enforce the usage of useCanvasState */}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 👍

@balazsbajorics balazsbajorics merged commit 7591c49 into master Jul 19, 2024
13 checks passed
@balazsbajorics balazsbajorics deleted the fix/use-editor-state-inside-canvas branch July 19, 2024 12:56
balazsbajorics added a commit that referenced this pull request Jul 19, 2024
balazsbajorics added a commit that referenced this pull request Jul 19, 2024
liady pushed a commit that referenced this pull request Dec 13, 2024
**Problem:**
When anything in the projectContents changes, the DerivedState.remixData
gets recreated. (This should be improved to only recreate the remixData
if we actually change the file-based routing)

The `UtopiaRemixRootComponent` uses **illegal** (🙂) useEditorState hooks
to access `DerivedState.remixData`.

Since `DerivedState.remixData` is referentially new, the useEditorState
hooks will trigger a re-render of every UtopiaRemixRootComponent.

The UtopiaRemixRootComponent rerenders then basically trigger an almost
full rerender of the canvas before the "real" canvas rerender step.

on master:
<img width="1097" alt="image"
src="https://github.com/user-attachments/assets/d15c8a69-0bbc-47de-be08-c896b8e70f1c">
with Sean's createExecutionScope cache fix (currently disabled and
blocked by #6091):
<img width="849" alt="image"
src="https://github.com/user-attachments/assets/e649e8da-8b1b-43f7-b207-927d67bcfbbf">

On the image you can see that in the sample store's `/` route this takes
15ms every single frame.

**Fix:**
**20ms -> 10ms -> 5ms**
<img width="885" alt="image"
src="https://github.com/user-attachments/assets/c78e1ec7-993f-4bc7-b109-8030bd649e5c">
When the canvas is in selective rerender mode, DO NOT let
`UtopiaRemixRootComponent` rerender itself. Not updating
UtopiaRemixRootComponent during the selective mode should be safe, since
the continuous canvas interactions never cause the file based routing or
loaders etc to update.

**Commit Details:** (< vv pls delete this section if's not relevant)

- Wrapping the canvas in a EditorStateContext provider with the value
forced to `null` which means we will throw an error in the future every
time new code accidentally uses the useEditorState hooks (which are
_not_ compatible with selective re-rendering). Hopefully this means
selective re-rendering will be less brittle in the future, but in a
follow up PR I will write tests for this too
- Added new hook `useCanvasState` which is almost the same as
`useEditorState` except it takes an extra callback `allowRerender`. If
AllowRerender is false, then no matter what changes in the editor state,
we will passively reject the component update and the hook will return
the _old_ state value.
- Using the new hook in `utopia-remix-root-component`
- Profit!

**Notes:**
This PR depends on #6091 because the selective re-rendering of the remix
root component causes the same css issue as Sean's original #6076

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode
liady pushed a commit that referenced this pull request Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants