-
-
Notifications
You must be signed in to change notification settings - Fork 139
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: client build css #549
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean #452 issue has already been resolved with previous changes?
Please check prettier format error.
btw, I remember I did the similar one in #456 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reconsider to do tasks in vite plugins.
publicIndexHtml = publicIndexHtml.replace(/<\/head>/, `${css}</head>`); | ||
await writeFile(publicIndexHtmlFile, publicIndexHtml); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really bad. Too hacky. It should already be modified in advance. Can you do things in rscIndexPlugin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried this, but could not, rollup should finish analyzing and creating an output. Then we'd know if there was any CSS emitted. What do you think? unless we create another build just for analyzing (which is too much as it's a complete build in itself)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do it in rscAnalyzePlugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No:
1- We haven't found the clientEntries even yet. That's the plugin's job to do so.
2- We stop walking the client files anyway. So it won't be reliable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we need to 2-pass analyze anyway. So, let's wait this PR until then, or we do the 2-pass analyze in advance (in a different PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do it in another PR after #552
@@ -518,12 +533,10 @@ const emitHtmlFiles = async ( | |||
moduleIdsForPrefetch, | |||
) + (customCode || ''); | |||
if (code) { | |||
const str = `<script type="module" async>${code}</script>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str
is too naive naming. scriptTagStr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea thought the same
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aslemammad How does #658 relate to this and #452? |
#658 (comment) Yeah, I had the same comment. 😄 |
haha yea, we need to do the 2 pass thing as mentioned! |
Resolves #452
For Server build (not ssr), the determined css was being injected using cssAssets into the html when building the ssr/client output. That's why rsc css used to work in build.
The issue is when the client output was emitted, the css was generated too. But since vite does not find that css being generated by the index.html/main.tsx, rather by other entries, it didn't include it in the final index.html output.
The solution is to get that generated css and inject it manually into the final html file.