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 package dependencies #1489

Merged
merged 10 commits into from
Nov 7, 2023
Merged

Fix package dependencies #1489

merged 10 commits into from
Nov 7, 2023

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Nov 6, 2023

Trying to fix peer dependencies of our packages here. See comments below.

Comment on lines +1 to +13
---
'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"
...
}
```
Copy link
Contributor Author

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.

Copy link
Contributor

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 🙈

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 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.

Comment on lines -62 to -64
"@remix-run/react": "^2.1.0",
"@shopify/hydrogen": "2023.10.2",
"@shopify/remix-oxygen": "^2.0.0"
Copy link
Contributor Author

@frandiox frandiox Nov 6, 2023

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.

Comment on lines -74 to -80
"peerDependenciesMeta": {
"@remix-run/server-runtime": {
"optional": true
},
"@remix-run/react": {
"optional": true
}
Copy link
Contributor Author

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.

Comment on lines -49 to -52
"peerDependenciesMeta": {
"@remix-run/server-runtime": {
"optional": true
}
Copy link
Contributor Author

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",
Copy link
Contributor Author

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?

Copy link
Contributor

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.

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.

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.

@frandiox frandiox merged commit 8fce70d into main Nov 7, 2023
9 checks passed
@frandiox frandiox deleted the fd-fix-deps branch November 7, 2023 02:56
@frandiox
Copy link
Contributor Author

frandiox commented Nov 7, 2023

It seems to be working well in @next:

image
╰─❯ npm run typecheck                

> hydrogen-test@0.0.0-next-8fce70d-20231107025821 typecheck
> tsc --noEmit

I've tried virtual routes, dev and build with npm and pnpm because they both organize node_modules in different ways. It seems to be working well 👍

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