fix(remix-dev/vite): keep code-split JS files in SSR build

[hi-ogawa]

Closes: #8041
Follow up for #7892

EDIT: following comment is for initial approach. Now it's written to remove only unnecessary files based on the suggestion in #8042 (comment)

---

Instead of keeping whole assets directory, maybe we could probe vite manifest and keep only isDynamicEntry.
(EDIT: I further extended reproduction https://github.com/hi-ogawa/remix-reproduction-8041 and I found that isDynamicEntry is not a right way to detect this)

For example, vite manifest from ssr build for vite-build-test.ts looks like this:

reveal build/.vite/manifest.json

(here assets/test2-LxNzCzR4.txt is server-only assets, so it's moved to client assets directory. assets/test1-eT52RzBS.txt and assets/_virtual_server-entry-119NVDiu.css are unnecessary files to keep.)

{
  "app/assets/test1.txt": {
    "file": "assets/test1-eT52RzBS.txt",
    "src": "app/assets/test1.txt"
  },
  "app/assets/test2.txt": {
    "file": "assets/test2-LxNzCzR4.txt",
    "src": "app/assets/test2.txt"
  },
  "app/ssr-code-split-lib.ts": {
    "file": "assets/ssr-code-split-lib-l8BW9uKL.js",
    "isDynamicEntry": true,
    "src": "app/ssr-code-split-lib.ts"
  },
  "virtual:server-entry": {
    "assets": [
      "assets/test1-eT52RzBS.txt",
      "assets/test2-LxNzCzR4.txt"
    ],
    "css": [
      "assets/_virtual_server-entry-119NVDiu.css"
    ],
    "dynamicImports": [
      "app/ssr-code-split-lib.ts"
    ],
    "file": "index.js",
    "isEntry": true,
    "src": "virtual:server-entry"
  }
}

However, I don't think it's so important to clean up the rest so strictly (and also I'm not entirely sure if there could be more edge cases which I'm not aware of). So, I chose to keep the directory as is.
Please let me know what you think!

[changeset-bot]

🦋 Changeset detected

Latest commit: c6813bf

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

[hi-ogawa]

For consistency, I also changed from move to copy so that ssr build assets will be kept intact.

[markdalgleish]

@hi-ogawa Thanks for looking into this!

I don't think it's so important to clean up the rest

I'd prefer for us to aim for a clean server build output so that people aren't deploying a tonne of unused assets along with their server code. Especially when we're still marked as unstable and have a bit of leeway to get this right.

Maybe a more safe approach, rather than removing the entire assets directory, is to do a more targeted removal of all assets and css files in the server manifest?

[hi-ogawa]

I'd prefer for us to aim for a clean server build output so that people aren't deploying a tonne of unused assets along with their server code. Especially when we're still marked as unstable and have a bit of leeway to get this right.

Thanks for the feedback!
Right, it makes sense to still experiment to strive for better while it's unstable.

The another concern I'm thinking about is that, for experienced vite users (who are familiar with manifest: true and ssrEmitAssets: true) or other vite plugins making use of the same options, it might be simply unexpected for some assets to disappear which vite would usually emit.

This might be out of scope right now, but for example, for whatever reasons, such users (or other plugins) might wish to customize rollupOptions.input/output during ssr build to include more server entry files, which then might utilize manifest.json and assets files, which they might not want for remix to delete.
So, deleting things could take away the flexibility of vite plugin system, which I think is a strong aspect of vite ecosystem.

But, I totally understand that, for the most of use cases, this won't be an issue and remix doesn't want to bloat default server output which users are likely consume and deploy directly. Also probably removing chunk.assets and chunk.css files won't hurt anything, so I'll try that approach.

[markdalgleish]

You're raising fair concerns for sure. I think we aim for a clean build output for now, but we can always add an option to disable this behaviour if it turns out it's a legitimate issue for someone.

[hi-ogawa]

I updated a PR to cleanup unnecessary chunk.assets and chunk.css from server assets directory and also added a test case to verify it.
I would appreciate next round of reviews. Thanks!

[github-actions]

🤖 Hello there,

We just published version 2.3.1-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!

[github-actions]

🤖 Hello there,

We just published version 2.3.1 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!