-
Notifications
You must be signed in to change notification settings - Fork 172
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
#13441 Bundle Size — 62.64MiB (~+0.01%).
Warning Bundle contains 70 duplicate packages – View duplicate packages Bundle metrics
|
Current #13441 |
Baseline #13440 |
|
---|---|---|
Initial JS | 45.71MiB (~+0.01% ) |
45.7MiB |
Initial CSS | 0B |
0B |
Cache Invalidation | 21.54% |
21.52% |
Chunks | 31 |
31 |
Assets | 34 |
34 |
Modules | 4376 (+0.05% ) |
4374 |
Duplicate Modules | 524 (+0.19% ) |
523 |
Duplicate Code | 31.69% |
31.69% |
Packages | 469 |
469 |
Duplicate Packages | 70 |
70 |
Bundle size by type 2 changes
1 regression
1 improvement
Current #13441 |
Baseline #13440 |
|
---|---|---|
JS | 62.62MiB (~+0.01% ) |
62.62MiB |
HTML | 11.16KiB (-0.33% ) |
11.2KiB |
Bundle analysis report Branch fix/use-editor-state-inside-canv... Project dashboard
ruggi
approved these changes
Jul 18, 2024
</CanvasContainer> | ||
</UtopiaProjectCtxAtom.Provider> | ||
</RerenderUtopiaCtxAtom.Provider> | ||
{/* deliberately breaking useEditorState and useRefEditorState to enforce the usage of useCanvasState */} |
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.
👍 👍
bkrmendy
approved these changes
Jul 18, 2024
gbalint
approved these changes
Jul 18, 2024
balazsbajorics
added a commit
that referenced
this pull request
Jul 19, 2024
This reverts commit 7591c49.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 accessDerivedState.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:
with Sean's createExecutionScope cache fix (currently disabled and blocked by #6091):
On the image you can see that in the sample store's
/
route this takes 15ms every single frame.Fix:
20ms -> 10ms -> 5ms
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)
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 toouseCanvasState
which is almost the same asuseEditorState
except it takes an extra callbackallowRerender
. 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.utopia-remix-root-component
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: