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

Move Vite plugins to their new home #1935

Merged
merged 44 commits into from
Apr 8, 2024
Merged

Move Vite plugins to their new home #1935

merged 44 commits into from
Apr 8, 2024

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Apr 2, 2024

This PR stabilizes our Vite plugins and moves them to their respective packages.

Originally, we were adding DX utilities (subrequest profiler, graphiql, etc.) directly in our CLI because the classic Remix compiler did not offer much flexibility.
When we released the experimental Vite integration, these utilities were adjusted to work with Vite but were still all in the CLI for the sake of velocity.

In this PR, these features are now migrated to the Vite plugins so that we stop using most of our hacky code around Remix to make them work.

Changes:

  • Move hydrogen() plugin to @shopify/hydrogen/vite. Vite is an optional peer dependency of this package now.
  • Move oxygen() plugin to @shopify/mini-oxygen/vite, which now contains all the workerd integration.
  • Migrate our virtual routes from the CLI to the Hydrogen plugin. Some code is left in the CLI to keep support for the classic Remix compiler, but the Vite plugin is now the source of truth.
  • Make the Hydrogen plugin work without Oxygen. This means users deploying to Node.js can still use our virtual routes.
  • Deprecated the code in @shopify/remix-oxygen for /subrequest-profiler. The new code is directly added in the Hydrogen plugin.

Copy link
Contributor

shopify bot commented Apr 2, 2024

Oxygen deployed a preview of your fd-stable-vite-plugins branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment April 8, 2024 6:40 PM
skeleton ✅ Successful (Logs) Preview deployment Inspect deployment April 8, 2024 6:40 PM
optimistic-cart-ui ✅ Successful (Logs) Preview deployment Inspect deployment April 8, 2024 6:40 PM
subscriptions ✅ Successful (Logs) Preview deployment Inspect deployment April 8, 2024 6:40 PM
custom-cart-method ✅ Successful (Logs) Preview deployment Inspect deployment April 8, 2024 6:40 PM
vite ✅ Successful (Logs) Preview deployment Inspect deployment April 8, 2024 6:40 PM

Learn more about Hydrogen's GitHub integration.

frandiox and others added 5 commits April 5, 2024 19:11
* Use Remix presets for virtual routes

* Pin oxygen-cli dependency to fix installation

* Avoid adding virtual routes in build

* Wrap virtual routes in virtual root

* Omit user root when rendering virtual routes

* Move buildDirectory to preset and use it in projects

* Update tests
@wizardlyhel
Copy link
Contributor

wizardlyhel commented Apr 5, 2024

I wonder if this is a first load error kinda thing. Happens right after npm run dev on skeleton. After refreshing subrequest profiler, it works fine.

I tried:

  • Close any subrequest profiler tab, run npm run dev, open subrequest profiler tab
  • Leave subrequest profiler tab, run npm run dev, and let HMR kicks in to refresh

both generated this error on first load

Screenshot 2024-04-05 at 1 21 45 PM

@frandiox
Copy link
Contributor Author

frandiox commented Apr 8, 2024

I wonder if this is a first load error kinda thing. Happens right after npm run dev on skeleton. After refreshing subrequest profiler, it works fine

@wizardlyhel This is a bug in Remix that we fixed already: remix-run/remix#9194

Should be out in the next release but maybe I should silence this error in the meantime 🤔 -- It's only related to critical CSS for the subrequest profiler, so not very important.


Hidden in 48f54be

@frandiox frandiox requested a review from a team April 8, 2024 14:36
@frandiox frandiox marked this pull request as ready for review April 8, 2024 14:36
Copy link
Contributor

@wizardlyhel wizardlyhel left a comment

Choose a reason for hiding this comment

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

Ahh okay! I would prefer to silence this error if we can until the next Remix release with fix. Otherwise, it's confusing for those encounter this error when they first run npm run dev

Other than that, this PR is working fine for me

@frandiox
Copy link
Contributor Author

frandiox commented Apr 8, 2024

I'm reverting #1939 here due to errors in subdependencies.

@frandiox frandiox merged commit ebaf552 into main Apr 8, 2024
13 checks passed
@frandiox frandiox deleted the fd-stable-vite-plugins branch April 8, 2024 18:48
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.

2 participants