Skip to content
This repository was archived by the owner on Apr 6, 2023. It is now read-only.
/frameworkPublic archive

perf(vite): remove duplicate css links from rendered page when inlined - #7264

Merged
pi0 merged 2 commits into
mainfrom
perf/duplicate-css
Sep 7, 2022
Merged

perf(vite): remove duplicate css links from rendered page when inlined#7264
pi0 merged 2 commits into
mainfrom
perf/duplicate-css

Conversation

@danielroe

@danielroedanielroe commented Sep 5, 2022•
edited
Loading

Copy link
Copy Markdown
Member

🔗 Linked issue

resolvesnuxt/nuxt#14792

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This improves the performance of inlining styles in SSR. We can complete remove styles from the manifest for individual/non-entry chunks, as we always know that these will be inlined. For entry CSS, it's a little more complex. Client-only components may have their CSS in the entry file, so we can't entirely exclude entry CSS. (And we deliberately tree shake client-only components from the server build.) I have lowered entry CSS priority via prefetching it; vite's preload helper then loads it.

Note: I think this is soluble, but it may require a secondary PR, which will also need to stop vite from preloading the entry css...

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroedanielroe added enhancement New feature or request 🍰 p2-nice-to-have Priority 2: nothing is broken but it's worth addressing labels Sep 5, 2022
@danielroe
danielroe requested a review from pi0September 5, 2022 22:33
@danielroedanielroe self-assigned this Sep 5, 2022
@netlify

netlifyBot commented Sep 5, 2022•
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for nuxt3-docs ready!

NameLink
🔨 Latest commit3b87c19
🔍 Latest deploy loghttps://app.netlify.com/sites/nuxt3-docs/deploys/6318582eca81820009e0a3d6
😎 Deploy Previewhttps://deploy-preview-7264--nuxt3-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

pi0
pi0 reviewed Sep 6, 2022
View reviewed changes
Comment threadpackages/vite/src/server.ts
ctx.nuxt.hook('build:manifest', (manifest) => {
for (const key in manifest) {
const entry = manifest[key]
const shouldRemoveCSS = chunksWithInlinedCSS.has(key)

@pi0pi0Sep 6, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen for a component with ssr inline syle enabled when rendering in hybrid/spa mode?

@danielroedanielroeSep 6, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rendering in hybrid/spa mode would not be adding a <link> for the CSS that is specific to the page anyway (as ssrContext.modules would be empty). So there would be no change from these lines (L127-133).

However, there would be a change to the entry CSS, which would be prefetched and then loaded on client-side rather than preloaded. (This would have a negative performance impact.)

- <link rel="preload" as="style" href="/_nuxt/entry.6e3e3577.css">- <link rel="stylesheet" href="/_nuxt/entry.6e3e3577.css">+ <link rel="prefetch stylesheet" href="/_nuxt/entry.6e3e3577.css">

Note that this also has a negative impact for client-only components for SSR pages. We could disable L135-R143, and though we would have double-loaded entry CSS values, we would not have any negative perf impact for client-side/or hybrid spa:

https://github.com/nuxt/framework/pull/7264/files#diff-9659ec6cc10298d2e39de216bea87573d29efdb58b9fd2e8fc9fc4404719bcaaR135-R143

pi0
pi0 approved these changes Sep 7, 2022
View reviewed changes
@pi0pi0 changed the title perf(vite): remove duplicate .css from rendered pageperf(vite): remove duplicate css links from rendered page when inlinedSep 7, 2022
@pi0
pi0 merged commit 577a7b6 into mainSep 7, 2022
@pi0
pi0 deleted the perf/duplicate-css branch September 7, 2022 08:41
@DamianGlowala

DamianGlowala commented Sep 7, 2022

Copy link
Copy Markdown
Member

I've tracked the PR which breaks my existing project and this one does it. There's an infinite refresh of the page with 'Reloading server...' message and a console log (undefined) in the terminal. Will provide any additional info if needed.

image
image

@harlan-zwharlan-zw mentioned this pull request Sep 8, 2022
Closed
@harlan-zw

harlan-zw commented Sep 8, 2022

Copy link
Copy Markdown
Collaborator

I haven't looked into it too much but something in RC.10 is causing this issue windicss/nuxt-windicss#180, this PR seems like a likely candidate.

Any ideas before I dig into it?

@pi0pi0 mentioned this pull request Sep 9, 2022
Merged
@danielroedanielroe mentioned this pull request Sep 9, 2022
Merged
7 tasks
@pi0pi0 mentioned this pull request Sep 15, 2022
Merged
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@pi0pi0pi0 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@danielroedanielroe

Labels

3.xenhancementNew feature or request🍰 p2-nice-to-havePriority 2: nothing is broken but it's worth addressing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

avoid double-downloading of inlined css

4 participants

@danielroe@DamianGlowala@harlan-zw@pi0