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): use ssrEmitAssets to support assets referenced by server-only code #7892

Merged
merged 10 commits into from
Nov 16, 2023

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 3, 2023

Closes: #7847

I'm not so sure how robust this change is (especially writeBundle plugin hook), but it think it should achieve mostly same thing as what sveltekit did in sveltejs/kit#9073.
See this comment #7847 (comment) for more references (especially vite's "stabilizing" discussion vitejs/vite#13808).

I added a test only for build case. I think this is not relevant for dev since vite serves assets files directly without moving or renaming file name with content hash etc..., so I didn't add a test.
Please let me know if there are others cases to be covered by tests.

  • Docs
  • Tests

Testing Strategy:

Copy link

changeset-bot bot commented Nov 3, 2023

🦋 Changeset detected

Latest commit: d85fd10

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

Comment on lines 482 to 485
// after ssr asset generation, move them to client assets directory
// https://rollupjs.org/plugin-development/#writebundle
Copy link
Contributor Author

@hi-ogawa hi-ogawa Nov 3, 2023

Choose a reason for hiding this comment

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

As indicated in vitejs/vite#11430 (comment), one thing this approach could be lacking is that vite's client manifest.json won't include entries from ssr assets.
I saw vite manifest is used in createBuildManifest/resolveBuildAssetPaths, but this might not be a critical issue since ssr assets are not something hard-coded by client routes and I don't think it's useful for the purpose of per-route resource pre-fetching etc...


However, if users wish to do their own post-processing based on manifest.json after vite build && vite build --ssr, then it may be desired to merge client and server manifest.json in some way.

@MichaelDeBoey MichaelDeBoey changed the title fix(vite): use ssrEmitAssets to support assets files referenced by server only code fix(remix-dev/vite): use ssrEmitAssets to support assets files referenced by server only code Nov 6, 2023
@markdalgleish markdalgleish self-assigned this Nov 14, 2023
Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

@hi-ogawa Thanks for the PR! I pushed a big update with the following changes:

  • We now only move SSR assets if they're not already present in the client build
  • We log to the terminal which SSR-only assets were moved so consumers can see what's happening
  • We clean up the SSR assets dir afterwards since the server doesn't need it (and in my testing it also contains built, unminfied CSS too)

I'm keen to hear what you think!

Copy link
Contributor Author

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

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

Nice! The logging looks good and it would definitely help not just for users but also for the sake of debugging when there is something wrong with vite ssr assets.

I added a few minor comments and I'll fix up my own typo, but other than that, feel free to take over the PR and merge!

.changeset/tricky-frogs-film.md Outdated Show resolved Hide resolved
integration/vite-build-test.ts Outdated Show resolved Hide resolved
packages/remix-dev/vite/plugin.ts Outdated Show resolved Hide resolved
packages/remix-dev/vite/plugin.ts Outdated Show resolved Hide resolved
packages/remix-dev/vite/plugin.ts Outdated Show resolved Hide resolved
markdalgleish and others added 2 commits November 16, 2023 11:28
Co-authored-by: Hiroshi Ogawa <hi.ogawa.zz@gmail.com>
Co-authored-by: Pedro Cattori <pcattori@gmail.com>
@markdalgleish markdalgleish dismissed pcattori’s stale review November 16, 2023 00:37

Changes have been addressed

@markdalgleish markdalgleish changed the title fix(remix-dev/vite): use ssrEmitAssets to support assets files referenced by server only code fix(remix-dev/vite): use ssrEmitAssets to support assets referenced by server-only code Nov 16, 2023
@markdalgleish markdalgleish merged commit bdc6004 into remix-run:dev Nov 16, 2023
9 checks passed
markdalgleish added a commit that referenced this pull request Nov 16, 2023
… by server-only code (#7892)

Co-authored-by: Mark Dalgleish <mark.john.dalgleish@gmail.com>
Co-authored-by: Pedro Cattori <pcattori@gmail.com>
@hi-ogawa hi-ogawa deleted the fix-vite-ssr-emit-assets branch November 17, 2023 12:05
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.

5 participants