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(remix-dev/vite): import hmr runtime from main script to ensure script ordering #7916

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 5, 2023

Closes: #7863

I saw that this issue seems to be addressed by #7842, but I still observed @vitejs/plugin-react can't detect preamble after reloading repeatedly like 10 or 20 times (see reproduction in #7863 (comment)), so it might be that "async" attribute doesn't necessarily guarantee execution order.

I was looking at the code and it seems pre-vite HMR had already some logic to inject extra import in <Scripts /> before running client entry, so I was wondering if there is any downside to use this for vite plugin as well:

manifest.hmr?.runtime
? `import ${JSON.stringify(manifest.hmr.runtime)};`

One clear difference is that this would setup HMR regardless of LiveReload, but currently if users didn't include LiveReload then they will get the same error @vitejs/plugin-react can't detect preamble anyway, so it seems this won't do any harm at least (though it may not be ideal eventually).

Testing Strategy:

Using the local build of this branch in hi-ogawa/test-remix-vite-hmr-runtime#2, I verified the same error doesn't happen on CI anymore (testing 1000 times reload).

Copy link

changeset-bot bot commented Nov 5, 2023

⚠️ No Changeset found

Latest commit: b87ce53

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

hi-ogawa added a commit to hi-ogawa/test-remix-vite-hmr-runtime that referenced this pull request Nov 5, 2023
@MichaelDeBoey MichaelDeBoey changed the title fix(vite): import hmr runtime from main script to ensure script ordering fix(remix-dev/vite): import hmr runtime from main script to ensure script ordering Nov 6, 2023
@MichaelDeBoey MichaelDeBoey force-pushed the fix-vite-hmr-runtime-script-dependency branch from 06856da to b87ce53 Compare November 6, 2023 01:13
@markdalgleish
Copy link
Member

markdalgleish commented Nov 6, 2023

@hi-ogawa Thanks for the PR! I had actually independently arrived at the same solution as part of #7919 which is pretty reassuring. I'm closing this PR in favour of that one since it goes a bit further, also fixing the issue where <LiveReload /> would cause an error if rendered after <Scripts />, and fixing the issue where it was an error to omit <LiveReload />.

@hi-ogawa hi-ogawa deleted the fix-vite-hmr-runtime-script-dependency branch November 6, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants