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

Serve assets from different domain in development #2078

Closed
wants to merge 10 commits into from

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented May 2, 2024

This serves assets in Vite like CSS / images from localhost:9100/mini-oxygen/00000/11111/22222/33333/... to mimic production, just like we used to do in the classic compiler.

However, JS assets generated from Remix are still served from localhost:3000/... because Remix does not check Vite's server.origin.

Therefore, for now at least, we only serve non-JS assets from a different domain, which is the main thing we want to mimic.

For the record, a partial fix in Remix would look like this:

// Use server.origin if available
let publicPath = (viteCommand === "serve" ? viteUserConfig.server?.origin ??  viteUserConfig.base  : viteUserConfig.base) ?? "/";

// Restore http:// after calling path.join in multiple places
let joinPaths = (...paths) => path.posix.join(...paths).replace(/^(https?):\/\/?/, '$1://');

That said, I'm not sure serving JS from a different domain is desirable because these are development chunks, which will be different in prod. Plus, the user probably doesn't need to point to JS chunks manually so there's not possible confusion / error to be made, unlike when pointing to images or CSS.


This works also for url(...) in CSS:

Prod build:
image

Development:
image


There's an issue with server.origin in Vite's HMR. I'm working on a separate PR to the Vite repo to fix that.
Right now, even though we serve files initially from 9100/mini-oxygen/extra-path, HMR for asset files start importing from 9100 root without keeping the path.

Copy link
Contributor

shopify bot commented May 9, 2024

Oxygen deployed a preview of your fd-vite-assets-domain branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
Skeleton (skeleton.hydrogen.shop) ✅ Successful (Logs) Preview deployment Inspect deployment May 20, 202410:05 AM
subscriptions ✅ Successful (Logs) Preview deployment Inspect deployment May 20, 202410:05 AM
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment May 20, 202410:05 AM
vite ✅ Successful (Logs) Preview deployment Inspect deployment May 20, 202410:05 AM
optimistic-cart-ui ✅ Successful (Logs) Preview deployment Inspect deployment May 20, 202410:05 AM
custom-cart-method ✅ Successful (Logs) Preview deployment Inspect deployment May 20, 202410:05 AM

Learn more about Hydrogen's GitHub integration.

@frandiox frandiox marked this pull request as ready for review May 20, 2024 10:04
@frandiox frandiox requested a review from a team May 20, 2024 10:06
@frandiox
Copy link
Contributor Author

I've come to the realization that serving assets from a different domain in development is not that useful anymore now that we are on Vite. The original reasons to do this is because the Classic Remix Compiler didn't have clever resolutions for assets, especially for CSS. Assets would work in development and later break in production so we had to "break" development as well. However, I think we are in a better place now:

  • In CSS, relative paths like background-image: url('../assets/image.svg') are resolved correctly by Vite from the path of the CSS file, and then when building for prod it resolves correctly from the base we give it with the CDN origin and pathname.
  • Same goes for absolute paths like url('/favicon.svg'). This was not processed in the classic compiler in production so it removed the pathname of the CDN. We had to "break" it in development to be the same. However, Vite correctly prefixes it with the origin + pathname of the CDN in prod build.
  • Our CSP is now including http://localhost:* so it wouldn't trigger CSP issues even when serving from a different port.

There are also a few cons to separating assets in a different server:

  • Remix does not support different origins for assets so all the JS chunks it links to on its manifest during development would not come from our separate domain unless we fix a few things there.
  • Since we would be adding a different server for assets, it would be harder to make it accessible from the network when using tunneling (--customer-account-push) or even Vite's --host flag to open it to the local network.
  • I think that the way to do this is not really "officially" supported in Vite. It just happens to work, most of it. I was going to open a PR to fix a minor issue in Vite itself but then realized this whole thing might not be a good idea anymore.

At first I thought the benefits (i.e. making dev more similar to prod) outweighed the cons but after realizing that the problems we had with the Classic Remix Compiler are now mostly solved... I think it's not worth the extra complexity this adds anymore.

Open to discussions, though, since I might be missing some of the benefits of serving from a different domain.

@frandiox frandiox closed this May 21, 2024
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.

None yet

1 participant