Fix changes from #310 to correctly collect imported CSS files

[XiNiHa]

This tries to land changes from #310 (that got rolled back in #312) without re-introducing dev FOUCs.
The problem was that even CSS imports don't have staticImportedUrls, which makes those files not collected as a dependency.
This PR adds a small check to see if nodes are CSS modules or not, to ensure all CSS imports are collected.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[changeset-bot]

⚠️ No Changeset found

Latest commit: fa2fc31

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[vercel]

@XiNiHa is attempting to deploy a commit to the Nikhil Saraf's projects Team on Vercel.

A member of the Team first needs to authorize it.

[katywings]

Why do you see the logic I wrote before was flawed?

I'll answer it here :D. Correct me if I am wrong, but from what I found (by debugging) staticImportedUrls does not relate to dynamic imports in any way shape or form 😅, even so the name might suggest that . staticImportedUrls just includes a list of all urls that are imported via code (including dynamic imports). With this information in mind your code would translate as follows:

Code: if (module.staticImportedUrls?.size || module.url.endsWith(".css"))
Translation: if [a file imports any other files] or [its url ends with .css] then walk through the file

If I am not completely wrong here, this logic does not solve the goal of excluding dynamically imported modules in the css crawling.

---

And here is the data from which I am basing on my understanding:

A module that is dynamically imported by its parent:

{
  url: '/src/components/box2.tsx',
  id: '/project/src/components/box2.tsx',
  file: '/project/src/components/box2.tsx',
  type: 'js',
  info: undefined,
  meta: undefined,
  importers: [
    '/project/src/routes/index.tsx?pick=default&pick=$css',
    '/project/src/routes/index.tsx'
  ],
  clientImportedModules: [
    '/node_modules/.vinxi/client/deps/solid-js_web.js?v=42960f6f',
    '/@solid-refresh',
    '/src/components/box2.module.css'
  ],
  ssrImportedModules: Set(0) {},
  acceptedHmrDeps: Set(0) {},
  acceptedHmrExports: null,
  importedBindings: null,
  isSelfAccepting: true,
  ssrTransformResult: null,
  ssrModule: null,
  ssrError: null,
  lastHMRTimestamp: 0,
  lastHMRInvalidationReceived: false,
  lastInvalidationTimestamp: 0,
  invalidationState: undefined,
  ssrInvalidationState: undefined,
  staticImportedUrls: Set(3) {
    '/node_modules/.vinxi/client/deps/solid-js_web.js?v=42960f6f',
    '/@solid-refresh',
    '/src/components/box2.module.css'
  },
  importedModules: [
    '/node_modules/.vinxi/client/deps/solid-js_web.js?v=42960f6f',
    '/@solid-refresh',
    '/src/components/box2.module.css'
  ]
}

A route component, that dynamically imports box2. Notice how staticImportedUrls includes box2 even though it is dynamically imported:

{
  url: '/project/src/routes/index.tsx?pick=default&pick=$css',
  id: '/project/src/routes/index.tsx?pick=default&pick=$css',
  file: '/project/src/routes/index.tsx',
  type: 'js',
  info: undefined,
  meta: undefined,
  importers: [],
  clientImportedModules: [
    '/node_modules/.vinxi/client/deps/solid-js_web.js?v=42960f6f',
    '/@solid-refresh',
    '/node_modules/.vinxi/client/deps/solid-js.js?v=42960f6f',
    '/src/components/box.tsx',
    '/src/routes/index.css',
    '/src/routes/index.module.css',
    '/src/components/box2.tsx'
  ],
  ssrImportedModules: Set(0) {},
  acceptedHmrDeps: Set(0) {},
  acceptedHmrExports: null,
  importedBindings: null,
  isSelfAccepting: true,
  ssrTransformResult: null,
  ssrModule: null,
  ssrError: null,
  lastHMRTimestamp: 0,
  lastHMRInvalidationReceived: false,
  lastInvalidationTimestamp: 0,
  invalidationState: undefined,
  ssrInvalidationState: undefined,
  staticImportedUrls: Set(7) {
    '/node_modules/.vinxi/client/deps/solid-js_web.js?v=42960f6f',
    '/@solid-refresh',
    '/node_modules/.vinxi/client/deps/solid-js.js?v=42960f6f',
    '/src/components/box.tsx',
    '/src/routes/index.css',
    '/src/routes/index.module.css',
    '/src/components/box2.tsx'
  },
  importedModules: [
    '/node_modules/.vinxi/client/deps/solid-js_web.js?v=42960f6f',
    '/@solid-refresh',
    '/node_modules/.vinxi/client/deps/solid-js.js?v=42960f6f',
    '/src/components/box.tsx',
    '/src/routes/index.css',
    '/src/routes/index.module.css',
    '/src/components/box2.tsx'
  ]
}

[katywings]

Ping @XiNiHa, see my answer above (I forgot to ping you ':)

[XiNiHa]

@katywings I see, I also checked that behavior by myself. Looks like we should just use what Vite provides (like deps and dynamicDeps included in the result of ViteDevServer.transformRequest?) I wonder why @nksaraf initially implemented this using module graph traversal (maybe for performance reasons) and want to know whether that still applies.

[katywings]

@XiNiHa Basically vite transforms the modules for server (ssr) and client differently. From what I can gather, vite only includes deps and dynamicDeps in the result of the ssr transform. As you can see on line

vinxi/packages/vinxi/lib/manifest/collect-styles.js

Line 104 in 02a3270

for (const url of node.ssrTransformResult.deps) {

, we already use deps on the ssr transform and exclude dynamicDeps. The client branch on the other hand uses a different tactic, because its transformResult doesnt have deps.

A look into the vite internals:

• Deps in ssrTransform: https://github.com/vitejs/vite/blob/fc049719053a3051179aa1e5f46889f2ee2832b1/packages/vite/src/node/ssr/ssrTransform.ts#L352
• No deps in client transform: https://github.com/vitejs/vite/blob/fc049719053a3051179aa1e5f46889f2ee2832b1/packages/vite/src/node/server/transformRequest.ts#L401

Now I don't know, why vite doesn't include deps in the client transform. I also don't know if we could somehow use the results from the ssr transform in the non-ssr branch, or if this would lead into bugs and flakiness 😅.

[XiNiHa]

I believe using results from SSR branch will have many problems in edge cases (DCE with import.meta.env.SSR, different entries per export conditions, probably more?) so should be avoided IMO.

[katywings]

Yupp thats also what I assume ☺️👍. So we still don't know a way to identify and exclude dynamic imports in the client transform 😅, or do you have a different idea :)?

[nksaraf]

if we get to know in prod, maybe in dev its okay that the css is eagerly prefetched, (since we don't know how to exclude it). I am surprised tho that the client side doesn't have any information.

[XiNiHa]

Actually it was the dev server performance that was problematic with me; I have a file that includes dynamic imports for all MDX files in the project, and importing that file resulted in transforming all the MDX files, which was very critical to dev performance.

[XiNiHa]

I think using es-module-lexer by ourselves can be a last resort, but it'd be better to look for more efficient approaches first 👀 looks like there are none! 😇

[katywings]

@XiNiHa Since you mentioned that it is a very specific file resulting in a performance degradation for you, how about this: instead of us trying to identify dynamic imports, how about we add a config to vinxi, where the user can exclude files from dev css crawling? So you could just exclude that one big file and we wouldnt have to analyze all nodes with the lexer 😅.

[XiNiHa]

That sounds very temporary 😂 I think the fundamental fix would be creating a patch on Vite to store static and dynamic dependencies separately and utilizing it. However I'm open to doing both!

Besides that, I'd also prefer to have some kind of flag comments (something like // @vinxi-ignore style-collection on the top?) instead of having them inside the config, since that would clearly show what is happening when looking at the file.

[katywings]

@nksaraf What would you prefer?

• A: Make a pr in vite, adding deps/dynamicDeps to non-ssr transform
• B: Add a way to exclude files from the css collection, either by config or comment
• C: Use the lexer to identify dynamic imports

Personally I am in favor of B, because:

• It avoids foucs in most projects
• It still gives consumers a way to optimize the css collection and introduce foucs, if they have to
• We dont have to make changes in the vite core
• It doesn't introduce a new perf bottleneck (lexer)

[nksaraf]

I like B, always favor these explicit hacks that give precise control. Is it safe/easy to do?

And yes, maybe A is ideal, but lets be practical and solve our problems on our end. Upstreaming is always an unpredictable process

[katywings]

@XiNiHa Would you be open to implement the file exclusion (via config or comment - whichever you prefer) and do you think its easy, or do you see risks?

If you will do it, can you make it in a new PR so that we can close this one 🙂?

Let me know if you wanna hear my arguments between config and comment 😁.

[XiNiHa]

I'd be happy to implement the feature, and I also want to know your thoughts between config and comment!
I see both options can be implemented pretty easily (fast-glob for configs, regexes for comments)
I'll close this PR and continue with a new one, but let's continue the conversation here until we move on!

[katywings]

@XiNiHa Awesome 🤩, thank you!

Regarding config vs comments I would say:

Comments:

• Makes sense if we assume that the solution is a temporary workaround and can be removed with future vite improvements
• Better visibility of the side-effect: you don't have to look into a separate config to see if the file you are working on is excluded
• The change has to touch fewer files, therefore probably easier to implement

Config:

• Makes sense if we want to indicate that the solution as a permanent feature of Vinxi
• Better visibility of the feature in docs
• Better performance: I assume excluding by filename is a smaller bottleneck, than excluding by filecontent, but I might be wrong 😅
• Automation: If the config supports arrays, the consumer could automate their exclude list and e.g. apply different excludes depending on env.

Both have their advantages and I see no clear winner. Whats your opinion @nksaraf ?

[katywings]

@XiNiHa Nikhil answered me personally: He prefers the exclusion via comment, in the same spirit as // @ts-ignore.

[XiNiHa]

👍 I'll go with // @vinxi-ignore-style-collection unless you prefer anything else!

[katywings]

@XiNiHa Sounds good to me 👍 and I guess we still can change it during the PR review, if Nikhil prefers something else :).