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

Better Remix Reset Handling. #6068

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Conversation

seanparsons
Copy link
Contributor

Problem:
In our sample project when correcting a typo in a route, it has been observed that the canvas doesn't update with the now fixed code.

Cause:
There are two main causes to this:

  • The previous logic that caught errors was only being triggered from the two error boundaries it had been written for.
  • Seemingly the canvas reset didn't work in this case, potentially because no other change was triggered, as the canvas store was updated before the mount count was bumped.

Fix:
The two causes were addressed like so:

  • A wrapper was implemented around useRouteError so that if it returns a non-null value, the pre-existing flag is set to true indicating an error happened.
  • The logic for resetting the canvas has been moved to a dispatched action which is fired alongside the action for updating the code editor.

Commit Details:

  • Removed handling of reset triggering from editorDispatchClosingOut.
  • Added useRouteError wrapper to createBuiltInDependenciesList.
  • reactRouterErrorTriggeredReset now returns a RESET_CANVAS action as required.
  • Incoming updates from the code editor can now trigger the canvas reset.

Manual Tests:
I hereby swear that:

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

- Removed handling of reset triggering from `editorDispatchClosingOut`.
- Added `useRouteError` wrapper to `createBuiltInDependenciesList`.
- `reactRouterErrorTriggeredReset` now returns a `RESET_CANVAS` action as
  required.
- Incoming updates from the code editor can now trigger the canvas reset.
Copy link
Contributor

github-actions bot commented Jul 11, 2024

Try me

Copy link

relativeci bot commented Jul 11, 2024

#13342 Bundle Size — 62.66MiB (~+0.01%).

49be9ee(current) vs 6339ece master#13341(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#13342
     Baseline
#13341
Regression  Initial JS 45.71MiB(~+0.01%) 45.71MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 22.28% 22.26%
No change  Chunks 30 30
No change  Assets 33 33
No change  Modules 4374 4374
No change  Duplicate Modules 524 524
No change  Duplicate Code 31.7% 31.7%
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
#13342
     Baseline
#13341
Regression  JS 62.65MiB (~+0.01%) 62.64MiB
Improvement  HTML 11.16KiB (-0.33%) 11.2KiB

Bundle analysis reportBranch fix/better-remix-reset-handlingProject dashboard

@seanparsons seanparsons merged commit 369e312 into master Jul 12, 2024
16 checks passed
@seanparsons seanparsons deleted the fix/better-remix-reset-handling branch July 12, 2024 14:07
liady pushed a commit that referenced this pull request Dec 13, 2024
- Removed handling of reset triggering from `editorDispatchClosingOut`.
- Added `useRouteError` wrapper to `createBuiltInDependenciesList`.
- `reactRouterErrorTriggeredReset` now returns a `RESET_CANVAS` action
as required.
- Incoming updates from the code editor can now trigger the canvas
reset.
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.

3 participants