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

Use Remix presets for virtual routes #1940

Merged
merged 9 commits into from
Apr 5, 2024

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Apr 3, 2024

This would allow us to remove a lot of hacky code to add virtual routes:

// vite.config.js
import {defineConfig} from 'vite';
import {hydrogen} from '@shopify/hydrogen/vite';
import {oxygen} from '@shopify/mini-oxygen/vite';
import {vitePlugin as remix} from '@remix-run/dev';
import tsconfigPaths from 'vite-tsconfig-paths';

export default defineConfig({
  plugins: [
    hydrogen(),
    oxygen(),
    remix({presets: [hydrogen.preset()]}),
    tsconfigPaths(),
  ],
});

However, there are currently some issues:

  • Small bug: Fix Vite critical CSS resolution for route files with absolute paths remix-run/remix#9194
  • I think all of the virtual routes would now go through the user's root loader. In the previous implementation, we could use a custom virtual root without any heavy requests. Therefore, virtual routes are probably slower now because user roots tend to contain queries for cart, header, etc. that we don't need. Is there any way to add a route and skip the root?
    • ✅ Update: found a hacky workaround 2443887
  • Not sure if it's possible to disable virtual routes with a CLI flag or when it's a build (instead of serving), since the Remix presets don't have access to the CLI flag or Vite info (I think?). This means we are now adding /graphiql and /subrequest-profiler to builds with this approach. We could probably just rely on process.env.NODE_ENV to at least avoid it in production builds.

Other thoughts:

  • It would be great if Remix exposed an API to add presets programmatically (Vite plugin's api: {}). That way we could add the preset only for development, and if the user doesn't pass --disable-virtual-routes to our CLI. Also, you wouldn't need to specify hydrogen.preset() in vite.config.js.

@frandiox
Copy link
Contributor Author

frandiox commented Apr 3, 2024

Thinking about this more, maybe we could create a secondary Remix app with only our virtual routes, and use the Node handler instead of workerd for this 🤔

Will check this idea soon.

Update: too many rabbit holes with this approach, going back to adding routes to the preset approach.

Copy link
Contributor

shopify bot commented Apr 4, 2024

Oxygen deployed a preview of your fd-preset-virtual-routes branch. Details:

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

Learn more about Hydrogen's GitHub integration.

@frandiox frandiox marked this pull request as ready for review April 5, 2024 11:38
Copy link
Contributor

github-actions bot commented Apr 5, 2024

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset.
If the changes are user-facing and should cause a version bump, run npm run changeset add to track your changes and include them in the next release CHANGELOG.
If you are making simple updates to examples or documentation, you do not need to add a changeset.

@frandiox frandiox merged commit f5cfa11 into fd-stable-vite-plugins Apr 5, 2024
13 checks passed
@frandiox frandiox deleted the fd-preset-virtual-routes branch April 5, 2024 18:09
frandiox added a commit that referenced this pull request Apr 8, 2024
* Move Oxygen plugin to mini-oxygen package

* Do not log favicon requests

* Ignore .shopify directory from all examples

* Move Hydrogen plugin to hydrogen package

* Add coverage check

* Pass plugin options using a public API

* Get plugin options in CLI for checks

* Flatten Oxygen plugin options

* Type safety for plugin API

* Simplify services in MiniOxygen

* Improve critical CSS code and docs

* Adapt subrequest profiler backend to Vite

* Replace setupScripts with crossBoundarySetup in MiniOxygen Vite

* Add Vite types to tsconfig.json

* Create __H2O_LOG_EVENT directly in Vite

* Update projects to new plugin paths

* Fix HMR

* Fix cross-package types

* Minor fixes to build types

* Fix virtual route test for classic compiler

* Fix type

* Add subrequest profiler logger globally in Node environments

* Improve types and split logic of mini-oxygen handler

* Replicate requestHook at the Vite Environment level

* Fix re-using request body

* Make the request hook be a POST request

* Move source of truth for sub-request profiler types to Hydrogen package

* Fix cross-package types

* Start sending subrequest-profiler events from Hydrogen middleware instead of remix-oxygen

* Support subrequest profiler in Node.js servers

* Use Remix presets for virtual routes (#1940)

* 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

* Use RemixConfig for appDir

* Hide non actionable error about critical CSS in virtual routes

* Improve types

* Read remix config from Hydrogen plugin

* Fix plugin types in vite.config.ts

* Changesets

* Remove unused internal feature

* Remove @experimental comment

* Dedup package-lock.json
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.

1 participant