-
Notifications
You must be signed in to change notification settings - Fork 67
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 changes from #310 to correctly collect imported CSS files #316
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
@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. |
I'll answer it here :D. Correct me if I am wrong, but from what I found (by debugging) Code: 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 {
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'
]
} |
Ping @XiNiHa, see my answer above (I forgot to ping you ':) |
@katywings I see, I also checked that behavior by myself. Looks like we should just use what Vite provides (like |
@XiNiHa Basically vite transforms the modules for server (ssr) and client differently. From what I can gather, vite only includes
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:
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 😅. |
I believe using results from SSR branch will have many problems in edge cases (DCE with |
Yupp thats also what I assume |
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. |
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. |
I think using |
@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 😅. |
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 |
@nksaraf What would you prefer?
Personally I am in favor of B, because:
|
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 |
@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 😁. |
I'd be happy to implement the feature, and I also want to know your thoughts between config and comment! |
@XiNiHa Awesome 🤩, thank you! Regarding config vs comments I would say: Comments:
Config:
Both have their advantages and I see no clear winner. Whats your opinion @nksaraf ? |
@XiNiHa Nikhil answered me personally: He prefers the exclusion via comment, in the same spirit as |
👍 I'll go with |
@XiNiHa Sounds good to me 👍 and I guess we still can change it during the PR review, if Nikhil prefers something else :). |
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.