-
Notifications
You must be signed in to change notification settings - Fork 274
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 package dependencies #1489
Fix package dependencies #1489
Conversation
--- | ||
'skeleton': patch | ||
--- | ||
|
||
Since Remix is now a peer dependency of `@shopify/remix-oxygen`, you need to add `@remix-run/server-runtime` to your dependencies with the same version you have for the rest of Remix dependencies. | ||
|
||
```diff | ||
"dependencies": { | ||
"@remix-run/react": "2.1.0" | ||
+ "@remix-run/server-runtime": "2.1.0" | ||
... | ||
} | ||
``` |
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.
@remix-run/server-runtime
was marked as a peer dependency in #1484. This is not what other Remix adapters do but I agree it makes sense for us since we don't follow the same versioning as other adapters.
The downside is that users should install @remix-run/server-runtime
as well as the adapter.
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 thought about this, but was worried about a breaking change. But we can just say this is apart of the 2023.10 release of making Remix a peer dep 🙈
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.
Yes this is actually a "bug fix". Luckily it doesn't even break anything because @remix-run/server-runtime
is already installed by other Remix packages I think.
"@remix-run/react": "^2.1.0", | ||
"@shopify/hydrogen": "2023.10.2", | ||
"@shopify/remix-oxygen": "^2.0.0" |
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.
These peer dependencies were added because we are using them in virtual routes. That might not be a big enough reason to list them here, let's just remove them for now since the project will execute in the context of the user project where these dependencies should already be installed.
We should test virtual routes in the @next
release to make sure this works.
"peerDependenciesMeta": { | ||
"@remix-run/server-runtime": { | ||
"optional": true | ||
}, | ||
"@remix-run/react": { | ||
"optional": true | ||
} |
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 don't think we should mark these as optional. The user should get an error/warning when installing if server-runtime or remix/react are not available since they are core to Hydrogen.
"peerDependenciesMeta": { | ||
"@remix-run/server-runtime": { | ||
"optional": true | ||
} |
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.
Similar to Hydrogen, server-runtime is core to the remix-oxygen adapter so there should be an error if it's missing.
"@shopify/cli": "3.50.0", | ||
"@shopify/cli-hydrogen": "^6.0.0", | ||
"@shopify/hydrogen": "^2023.10.0", | ||
"@shopify/remix-oxygen": "^2.0.0", | ||
"@shopify/hydrogen": "~2023.10.2", |
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.
Also, using ^
in our templates for Hydrogen leads to getting new major versions. Better to use ~
for this one, or even pin to our latest release every time. Thoughts?
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.
We just need to make sure a resolved project doesn't have duplicate remix dependencies.
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.
LGTM, let's merge and test the next
release. Specifically make sure that the types from useLoaderData<typeof loader>()
resolve properly. That's the server/client boundary that breaks when theirs a mismatch in the server/client runtime versions.
Trying to fix peer dependencies of our packages here. See comments below.