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

Support Remix HMR v2 #1187

Merged
merged 24 commits into from
Aug 18, 2023
Merged

Support Remix HMR v2 #1187

merged 24 commits into from
Aug 18, 2023

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Aug 4, 2023

Continues #652
Closes #886

@frandiox frandiox marked this pull request as ready for review August 4, 2023 15:13
@frandiox frandiox requested a review from a team August 4, 2023 15:13
@github-actions

This comment has been minimized.

@juanpprieto
Copy link
Contributor

I'm getting these which I guess is why I'm also seeing full refreshes instead of HMR

Screenshot 2023-08-14 at 11 05 26 AM

@frandiox
Copy link
Contributor Author

I'm getting these which I guess is why I'm also seeing full refreshes instead of HMR

How did you test it? These errors appear only when you stop the dev server so it might be unrelated to the problem you were seeing.

For example, if you go to /account/login in skeleton or demo-store and type something in the inputs, then change something in root.tsx#app, the layout in the browser should update but the input state shouldn't be reset. Is that happening or do you get a full refresh?

@github-actions github-actions bot had a problem deploying to preview August 15, 2023 10:45 Failure
@juanpprieto
Copy link
Contributor

I'm getting these which I guess is why I'm also seeing full refreshes instead of HMR

How did you test it? These errors appear only when you stop the dev server so it might be unrelated to the problem you were seeing.

For example, if you go to /account/login in skeleton or demo-store and type something in the inputs, then change something in root.tsx#app, the layout in the browser should update but the input state shouldn't be reset. Is that happening or do you get a full refresh?

All working well for me. The refresh was due to the dev:pkg command not running properly

@github-actions github-actions bot had a problem deploying to preview August 17, 2023 02:59 Failure
Copy link
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

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

A few minor comments. Looks good to me though.

Comment on lines +24 to +27
import('@remix-run/dev/dist/devServer_unstable/hmr.js'),
import('@remix-run/dev/dist/devServer_unstable/socket.js'),
import('@remix-run/dev/dist/devServer_unstable/hdr.js'),
import('@remix-run/dev/dist/result.js'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these other dependencies from Remix that we are hoping have unofficially stabilized?

Copy link
Contributor Author

@frandiox frandiox Aug 17, 2023

Choose a reason for hiding this comment

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

Yes, but kind of hoping that this whole live-reload logic, or something similar, can be added to Remix. It's basically an abstraction that we could import. But the devServer_unstable is still changing so we'll need to wait.

Like, most of this code comes from Remix itself, it's just mixed with other things there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we need to revive the conversation with them around these internal apis.

Comment on lines +52 to +53
const loaderChanges = await state.loaderChanges!;
if (loaderChanges.ok) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume there's not a scenario where loaderChanges is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

);
} else if (state.prevManifest) {
// Full reload
socket.reload();
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work, a full page reload? Why should this trigger here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Basically, there has been a change in the app but we can't track the change for some reason. Therefore, a full reload is the only option.

@frandiox frandiox merged commit d053978 into 2023-07 Aug 18, 2023
10 checks passed
@frandiox frandiox deleted the fd-hmr branch August 18, 2023 02:16
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.

Support for HMR
4 participants