refactor: mark a few client-side packages as side-effect-free #7085
Conversation
Josh-Cena
added
the
pr: performance
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-7085--docusaurus-2.netlify.app/ |
|
Size Change: -5.92 kB (-1%) Total Size: 797 kB
ℹ️ View Unchanged
|
|
The most prominent issue is CSS. As soon as a component imports CSS, to ensure stable CSS insertion order (#3678), we have to mark all downstream consumers as side-effect-ful. This makes us not able to actually fully tree-shake. |
| @@ -9,6 +9,8 @@ import React from 'react'; | |||
| import clsx from 'clsx'; | |||
| import {Details as DetailsGeneric} from '@docusaurus/theme-common'; | |||
| import type {Props} from '@theme/Details'; | |||
| // Ensure that the default details style is properly overridden | |||
| import '@docusaurus/theme-common/lib/components/Details/styles.module.css'; | |||
There was a problem hiding this comment.
We unfortunately have to do this, to make theme-common tree-shakeable at all. Maybe we should have different exports fields for all side-effect-ful exports (like @docusaurus/theme-common/Details), or simply declare this as the official way to acquire default styles for Details?
There was a problem hiding this comment.
🤷♂️ if we do this maybe we should remove the css module import in theme common?
Note useKeyboardNavigation also imports css
There was a problem hiding this comment.
Aye, but that one's not impacted by CSS insertion order, so as long as it is loaded (which, well, means useKeyboardNavigation.ts is used), it works properly.
My idea is that we should not export Details from src/index.tsx, but create a separate exports entry point instead.
There was a problem hiding this comment.
What I understand is that the CSS order problem is related to theme/Details vs theme-common/Details right?
This kind of problem is going to be solved soon once CSS cascade layers support improves (already quite good https://caniuse.com/css-cascade-layers)
@layer infima, theme-common, theme, site;In the meantime maybe we could just add !important to the 4 rules that override theme-common?
There was a problem hiding this comment.
Mmm, you can see I obviously don't like !important because that makes site customizations harder, if not impossible...
There was a problem hiding this comment.
yes agree, your solution is probably a better temporary workaround
Josh-Cena
added
the
status: awaiting review
|
This looks much better IMO, no more hacks... My idea is that |
Josh-Cena
removed
the
status: awaiting review
Motivation
Instant bundle size optimization⚡️
I did not mark core as side-effect-free, though, because there are a few side-effects there (mostly importing non-module CSS), and it's quite tedious trying to filter out everything.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Page builds fine