Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(remix-dev/vite): attach CSS from shared chunks to routes #7952

Merged
merged 7 commits into from
Nov 9, 2023

Conversation

hi-ogawa
Copy link
Contributor

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

Closes: #7941

I think the issue was due to asset resolution not taking account for code-split common assets.
I made a reproduction repo here https://github.com/hi-ogawa/test-remix-vite-css-split and this is my observation.
I attached vite manifest and remix manifest generated by build currently.
In vite manifest, _test-206a75ed.js entry has reference to assets/test-ef980167.css, but there's no direct css dependency neither from app/root.tsx or app/routes/_index.tsx, so remix manifest ends up with no css.

build/manifest.json (vite manifest)
{
  "_components-be9f5b54.js": {
    "file": "assets/components-be9f5b54.js",
    "imports": [
      "_jsx-runtime-26afeca0.js"
    ]
  },
  "_jsx-runtime-26afeca0.js": {
    "file": "assets/jsx-runtime-26afeca0.js"
  },
  "_test-206a75ed.js": {
    "css": [
      "assets/test-ef980167.css"
    ],
    "file": "assets/test-206a75ed.js",
    "imports": [
      "_jsx-runtime-26afeca0.js"
    ]
  },
  "app/root.tsx": {
    "file": "assets/root-42156807.js",
    "imports": [
      "_jsx-runtime-26afeca0.js",
      "_test-206a75ed.js",
      "_components-be9f5b54.js"
    ],
    "isEntry": true,
    "src": "app/root.tsx"
  },
  "app/routes/_index.tsx": {
    "file": "assets/_index-01b79c10.js",
    "imports": [
      "_jsx-runtime-26afeca0.js",
      "_test-206a75ed.js"
    ],
    "isEntry": true,
    "src": "app/routes/_index.tsx"
  },
  "node_modules/.pnpm/@remix-run+dev@0.0.0-nightly-8543efe-20231108_@remix-run+serve@0.0.0-nightly-8543efe-20231108_fetfjcoiwibmio4yfoqtdictta/node_modules/@remix-run/dev/dist/config/defaults/entry.client.tsx": {
    "file": "assets/entry.client-b45bdf04.js",
    "imports": [
      "_jsx-runtime-26afeca0.js",
      "_components-be9f5b54.js"
    ],
    "isEntry": true,
    "src": "node_modules/.pnpm/@remix-run+dev@0.0.0-nightly-8543efe-20231108_@remix-run+serve@0.0.0-nightly-8543efe-20231108_fetfjcoiwibmio4yfoqtdictta/node_modules/@remix-run/dev/dist/config/defaults/entry.client.tsx"
  },
  "test.css": {
    "file": "assets/test-ef980167.css",
    "src": "test.css"
  }
}
build/manifest-(hash).js (remix manifest)
window.__remixManifest = {
  entry: {
    module: "/build/assets/entry.client-b45bdf04.js",
    imports: [
      "/build/assets/jsx-runtime-26afeca0.js",
      "/build/assets/components-be9f5b54.js",
    ],
    css: [],
  },
  routes: {
    root: {
      id: "root",
      path: "",
      hasAction: false,
      hasLoader: false,
      hasErrorBoundary: false,
      module: "/build/assets/root-42156807.js",
      imports: [
        "/build/assets/jsx-runtime-26afeca0.js",
        "/build/assets/test-206a75ed.js",
        "/build/assets/components-be9f5b54.js",
      ],
      css: [],
    },
    "routes/_index": {
      id: "routes/_index",
      parentId: "root",
      index: true,
      hasAction: false,
      hasLoader: false,
      hasErrorBoundary: false,
      module: "/build/assets/_index-01b79c10.js",
      imports: [
        "/build/assets/jsx-runtime-26afeca0.js",
        "/build/assets/test-206a75ed.js",
      ],
      css: [],
    },
  },
  url: "/build/manifest-04ec219c.js",
  version: "04ec219c",
};

I fixed the issue by traversing ViteManifest[key].imports to collect depending assets of given route entry.
(I had similar issue when I was experimenting my own plugin, so the code is roughly based on https://github.com/hi-ogawa/vite-plugins/blob/0591f50b53ebd6027653a0902b50a492820fbbab/packages/vite-glob-routes/src/react-router/features/preload/shared.ts#L50-L59)

I locally verified that this PR fixed my reproduction hi-ogawa/test-remix-vite-css-split#1 and also added a new test case for vite-build-test.ts.

  • Docs
  • Tests

Testing Strategy:

Copy link

changeset-bot bot commented Nov 9, 2023

🦋 Changeset detected

Latest commit: 134a248

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 hi-ogawa changed the title fix(remix-dev/vite): resolve code split assets from vite manifest (wip) fix(remix-dev/vite): resolve code split assets from vite manifest Nov 9, 2023
@hi-ogawa hi-ogawa marked this pull request as ready for review November 9, 2023 05:49
@markdalgleish markdalgleish self-requested a review November 9, 2023 06:12
@markdalgleish markdalgleish changed the title fix(remix-dev/vite): resolve code split assets from vite manifest fix(remix-dev/vite): attach CSS from shared chunks to routes Nov 9, 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.

Thanks for the PR!

@markdalgleish markdalgleish merged commit d437e79 into remix-run:dev Nov 9, 2023
@markdalgleish
Copy link
Member

@hi-ogawa Also, thanks for all your hard work helping us with the Vite plugin! I really appreciate it 🙏

@hi-ogawa hi-ogawa deleted the fix-vite-code-split-css branch November 10, 2023 00:34
@hi-ogawa
Copy link
Contributor Author

Thanks for the nice cleanup. I'm happy to help and also learn a lot from remix code base!

Copy link
Contributor

🤖 Hello there,

We just published version 2.3.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.3.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!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants