-
Notifications
You must be signed in to change notification settings - Fork 302
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
bump remix and vite #2784
bump remix and vite #2784
Conversation
This comment has been minimized.
This comment has been minimized.
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
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.
Great work! A few questions:
-
What are the implications for people making this change? Is upgrading just bumping the versions in package.json? Or are there other things that they must do?
-
Will the CLI still work with older versions of Vite? Or does upgrading Hydrogen to this release mean that they must also upgrade Vite?
No project code needs to be updated aside from the version bump
That's a good question .. for sure the |
Yea @blittle This is a breaking change. We will have to update minimum dependency on Remix, vite, and cli. Otherwise, it will break when people tries to run vite 5 project on latest cli. |
Oh wait! I correct myself. If projects on vite 5 somehow updates cli, as long as we fix the |
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.
Looks good overall, just added a couple of comments.
I pictured this update as part of moving from Remix to React Router. I guess that's not the case yet? When that time comes, there's some code to remove related to Remix classic bundler in our CLI.
runtime.moduleCache.invalidateDepTree([ | ||
// Module IDs are absolute from root | ||
update.path.replace(/^\.\//, '/'), | ||
...(update.ssrInvalidates ?? []), | ||
]); | ||
runtime.import(update.acceptedPath); |
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.
This could lead to tricky bugs in HMR. I would encourage testing HMR in different situations and see if something fails (both server and browser code).
Not saying this change is not correct, but this area was tricky to get right so it would be better to double check (if you haven't yet).
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.
Hmm .. you are right about this. However, HMR still works .. just that it runs multiple times.
If it was an edit from the skeleton template, the hot reload got triggered about 3-4 times.
If it was an edit from the hydrogen-react package, the hot reload got triggered multiple times -- browser page crashes due to not being able to find modules at some point during a HMR update ping and then recovers itself after another reload.
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.
I'm not sure how to get around this since runtime.moduleCache.invalidateDepTree
completely disappears from Vite 6 because they said it was redundant.
/snapit |
🫰✨ Thanks @wizardlyhel! Your snapshots have been published to npm. Test the snapshots by updating your "@shopify/cli-hydrogen": "0.0.0-snapshot-20250318154141",
"@shopify/hydrogen": "0.0.0-snapshot-20250318154141",
"@shopify/hydrogen-codegen": "0.0.0-snapshot-20250318154141",
"@shopify/mini-oxygen": "0.0.0-snapshot-20250318154141",
"@shopify/remix-oxygen": "0.0.0-snapshot-20250318154141"
|
/snapit |
🫰✨ Thanks @wizardlyhel! Your snapshots have been published to npm. Test the snapshots by updating your "@shopify/cli-hydrogen": "0.0.0-snapshot-20250319224903",
"@shopify/hydrogen": "0.0.0-snapshot-20250319224903",
"@shopify/hydrogen-codegen": "0.0.0-snapshot-20250319224903",
"@shopify/mini-oxygen": "0.0.0-snapshot-20250319224903",
"@shopify/remix-oxygen": "0.0.0-snapshot-20250319224903"
|
@frandiox I fixed the CSS parsing issue. It turns out we didn't need any of the HMR hooks anymore in vite 6. Remove the HMR hooks in the mini-oxygen vite plugin, the css parsing error also goes away. |
/snapit |
🫰✨ Thanks @wizardlyhel! Your snapshots have been published to npm. Test the snapshots by updating your "@shopify/cli-hydrogen": "0.0.0-snapshot-20250320155609",
"@shopify/hydrogen": "0.0.0-snapshot-20250320155609",
"@shopify/hydrogen-codegen": "0.0.0-snapshot-20250320155609",
"@shopify/mini-oxygen": "0.0.0-snapshot-20250320155609",
"@shopify/remix-oxygen": "0.0.0-snapshot-20250320155609"
|
/snapit |
🫰✨ Thanks @wizardlyhel! Your snapshots have been published to npm. Test the snapshots by updating your "@shopify/cli-hydrogen": "0.0.0-snapshot-20250320172256",
"@shopify/hydrogen": "0.0.0-snapshot-20250320172256",
"@shopify/hydrogen-codegen": "0.0.0-snapshot-20250320172256",
"@shopify/mini-oxygen": "0.0.0-snapshot-20250320172256",
"@shopify/remix-oxygen": "0.0.0-snapshot-20250320172256"
|
WHY are these changes introduced?
2.16.1-pre.2
@vitejs/plugin-react
for vite compatibility@vanilla-extract/vite-plugin
for vite compatibilityWHAT is this pull request doing?
HOW to test your changes?
Post-merge steps
Checklist