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(remix-dev/vite): change build output paths #8077

Merged
merged 8 commits into from
Nov 22, 2023

Conversation

markdalgleish
Copy link
Member

@markdalgleish markdalgleish commented Nov 21, 2023

This is a fix for #8039 (comment) and a better fix for #8023 and #8038.

See the changeset and updated docs for more details, but tl;dr:

  • The server is now compiled into build/server by default (rather than build)
  • The client is now compiled into build/client by default (rather than public/build)

This fixes an issue where the way Vite manages the public directory clashes with the way the existing Remix compiler works. So far we've been trying to get Vite to behave like the Remix compiler but it's causing issues since it's such a different model, so instead we're now leaning into the way Vite works.

As a bonus, I personally think the build output feels a lot cleaner too ✨

Copy link

changeset-bot bot commented Nov 21, 2023

🦋 Changeset detected

Latest commit: 98a2d86

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@justinwaite
Copy link
Contributor

Would this necessitate updating image sec references in the apps as well?

For example, given you had an image stored at public/images/my-image.jpg and a reference to it in a page <img src="/images/my-image.jpg"/>, what would need to change here (if anything?)

Would the recommended approach change to storing image assets in a app/assets directory and let vite bundle them and handle the asset paths?

import imageSrc from "~/assets/images/my-image.jpg"

...

<image src={imageSrc}/>

@gustavopch
Copy link
Contributor

@justinwaite Without this PR, you'd have to move your images from public to app. The PR is addressing this exact situation so you can keep your images in public and reference them directly by the path without having to import. You can get more context here: #8039 (comment).

let defaults: Partial<RemixVitePluginOptions> = {
serverBuildPath: "build/server/index.js",
assetsBuildDirectory: "build/client",
publicPath: "/",
Copy link
Contributor

@hi-ogawa hi-ogawa Nov 22, 2023

Choose a reason for hiding this comment

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

Nice change!
With this change of default publicPath from /build to /, I feel it became closer to what base vite config does (base default value is also / https://vitejs.dev/config/shared-options.html#base).

I think remix still doesn't support custom basename in general, but I was wondering if it makes sense to deprecate publicPath remix config and favor base option from user vite config at some point (which also somehow works on dev too hopefully).

For the reference:

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a really good point! I'll keep this in mind when looking at custom base support.

@markdalgleish markdalgleish merged commit a3a31a3 into dev Nov 22, 2023
9 checks passed
@markdalgleish markdalgleish deleted the markdalgleish/change-vite-output-paths branch November 22, 2023 01:42
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Nov 22, 2023
@remix-run remix-run deleted a comment from github-actions bot Nov 22, 2023
@markdalgleish
Copy link
Member Author

This is available in the latest nightly, currently 0.0.0-nightly-8c42697-20231122

@justinwaite @gustavopch @nicksrandall If you could all try it out and report back, that would be super helpful 🙏

@nicksrandall
Copy link
Contributor

@markdalgleish I have migrated and it is working. I like this approach.

@justinwaite
Copy link
Contributor

@markdalgleish I was able to fork my original stackblitz and migrate: https://stackblitz.com/edit/remix-run-remix-5qicaa?file=vite.config.ts

The setup in the stackblitz is to emulate how we would setup an app to run on a base path with how Remix works today. In this case, the basepath is /mybase. It requires that:

  • Assets (images, etc) be placed in public/mybase
  • Routes are explicitly prefixed with mybase (e.g. mybase._index.tsx or mybase.settings.tsx)
  • The publicPath is set to /mybase in the remix config in the vite plugin (no longer need to mess with assetsBuildDirectory).

Testing it out, the build directory does indeed get populated with client and server directories. The assets in the public directory are copied into the client build. Remix serve seems to gracefully handle everything. When I check the network logs, js bundles and images are being properly served from /mybase.

Just a couple of thoughts:

  1. I really like the new build output structure. Makes sense, it's cleaner, and one less directory for me to make sure my editor is ignoring properly (and one less entry into gitignore 😄 )
  2. I wish Vite dev emulated production more closely. When running Vite in dev mode, assets are under /mybase, but rather are scattered around /node_modules, /app, @id, @vite, etc. This really only becomes annoying when trying to proxy apps in local development, where I have to manage proxy entries for all of those unique root-level entrypoints. Perhaps this would be a better experience if the vite base option were supported natively within remix?

Thanks for the quick support on this one!

@gustavopch
Copy link
Contributor

@markdalgleish Working here as well!

Copy link
Contributor

github-actions bot commented Dec 5, 2023

🤖 Hello there,

We just published version 2.4.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants