-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
test: ensure consistent CSS ordering #6222
Changes from 6 commits
ef15220
bd2947b
1684b07
b08783b
3bbe21d
74f3e64
4f4f571
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/* Used to test CSS insertion order */ | ||
.test-marker-site-client-module { | ||
content: "site-client-module"; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import React from 'react'; | ||
import type {Props} from '@theme/Layout'; | ||
import Layout from '@theme-original/Layout'; | ||
|
||
// This component is only used to test for CSS insertion order | ||
import './styles.module.css'; | ||
|
||
export default function LayoutWrapper(props: Props): JSX.Element { | ||
return <Layout {...props} />; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/* Used to test CSS insertion order */ | ||
.test-marker-theme-layout { | ||
content: "theme-layout"; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
const path = require('path'); | ||
const fs = require('fs'); | ||
|
||
/* | ||
This verifies CSS ordering on the Docusaurus site itself, | ||
|
||
There are multiple ways to provide some CSS to Docusaurus | ||
and Docusaurus should guarantee a consistent CSS ordering over time | ||
|
||
See also | ||
- https://github.com/facebook/docusaurus/issues/3678 | ||
- https://github.com/facebook/docusaurus/pull/5987 | ||
|
||
TODO we should probably add a real e2e test in core instead of using our own website? | ||
Current solution looks good-enough for now | ||
|
||
*/ | ||
|
||
// TODO temporary, the current order is bad and we should change that | ||
const EXPECTED_MARKER_CLASSES = [ | ||
// Some class from Infima, in the order Infima declares them | ||
// this ensures we don't mess-up and re-order css from other libs | ||
'.markdown>h2', | ||
'.button--outline.button--active', | ||
':root', // TODO should be first | ||
'.DocSearch-Hit-content-wrapper', // TODO should not be there? | ||
'.navbar__title', | ||
'.test-marker-site-custom-css-shared-rule', // TODO should not be there! | ||
'.col[class*=col--]', // TODO should be after paddings | ||
'.padding-vert--xl', | ||
'.footer__link-item', // TODO should be last | ||
'.pagination__item', | ||
'.pills__item', | ||
'.tabs__item', | ||
|
||
// Test markers | ||
'.test-marker-site-custom-css-unique-rule', | ||
'.test-marker-site-client-module', | ||
'.test-marker-theme-layout', | ||
'.test-marker-site-index-page', | ||
'.DocSearch-Modal', | ||
]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lex111 @Josh-Cena as we can see the output CSS is currently in a weird order. Due to CSS optimizations, Infima classes are re-ordered. Site custom CSS classes are "merged" with Infima classes sharing similar rules and put at the very beginning. We end up with CSS classes from different places (site, client modules, Infima, theme...) being interleaved in an unpredictable way. I'm not sure if this is the correct thing to do in all cases, and I'd rather disable such optimizations, as it's better to do something that works than save a few Kb. Are these optimizations always guaranteed 100% safe? The main problem remains that site custom CSS should rather be last so that a site owner can more easily override existing CSS without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, if it's because of optimization it makes perfect sense, and that's the entire purpose of optimization: combine duplicated classes where it can. If you have two class names that are identical in every way, it will be safe to combine them. I wouldn't worry about that since there's nothing to override in this case either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have a bloated bundle size that I've been looking for ways to minimize (e.g. #3655 could be a way out). Optimizations are meant to be optimizations: there aren't behavior differences but you can't expect everything to be the same, in the same way you can't rely on |
||
|
||
const cssDirName = path.join(__dirname, 'build', 'assets', 'css'); | ||
|
||
const cssFileNames = fs | ||
.readdirSync(cssDirName) | ||
.filter((file) => file.endsWith('.css')); | ||
|
||
if (cssFileNames.length !== 1) { | ||
throw new Error('unexpected: more than 1 css file'); | ||
} | ||
const cssFile = path.join(cssDirName, cssFileNames[0]); | ||
|
||
console.log('Inspecting CSS file for test CSS markers', cssFile); | ||
|
||
const cssFileContent = fs.readFileSync(cssFile, 'utf8'); | ||
|
||
const cssMarkersWithPositions = EXPECTED_MARKER_CLASSES.map((marker) => { | ||
const position = cssFileContent.indexOf(marker); | ||
return {marker, position}; | ||
}); | ||
|
||
const missingCSSMarkers = cssMarkersWithPositions | ||
.filter((m) => m.position === -1) | ||
.map((m) => m.marker); | ||
|
||
if (missingCSSMarkers.length > 0) { | ||
throw new Error( | ||
`Some expected CSS marker classes could not be found in file ${cssFile}: \n- ${missingCSSMarkers.join( | ||
'\n- ', | ||
)}`, | ||
); | ||
} | ||
|
||
// https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_sortby-and-_orderby | ||
const sortBy = (key) => (a, b) => | ||
// eslint-disable-next-line no-nested-ternary | ||
a[key] > b[key] ? 1 : b[key] > a[key] ? -1 : 0; | ||
|
||
const sortedCSSMarkers = cssMarkersWithPositions | ||
.concat() | ||
.sort(sortBy('position')) | ||
.map(({marker}) => marker); | ||
|
||
if ( | ||
JSON.stringify(sortedCSSMarkers) === JSON.stringify(EXPECTED_MARKER_CLASSES) | ||
) { | ||
console.log(`Test CSS markers were found in the expected order: | ||
- ${sortedCSSMarkers.join('\n- ')}`); | ||
} else { | ||
throw new Error(`Test CSS markers were found in an incorrect order. | ||
|
||
Expected order: | ||
- ${EXPECTED_MARKER_CLASSES.join('\n- ')}; | ||
|
||
Actual order: | ||
- ${sortedCSSMarkers.join('\n- ')}; | ||
|
||
CSS file: ${cssFile} | ||
`); | ||
} |
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.
...why is that?
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.
Imagine the following Infima CSS:
Imagine this element:
Now a site owner wants to override some CSS with custom site CSS:
User should expect the item to be 100% width, with the following CSS:
Now it's not what happens in practice, and the override does not work, because CSS is optimized as:
.ifm-half-width
still wins as it's last in the stylesheet, and the CSS optimization messed-up with the intent of the user.If the site owner chose a unique value, like "width: 99%" instead of a duplicate value like 100%, this would be applied.
Does it make sense?
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.
Yes, I was just missing the click word "optimization" there. You mentioned optimization below and it all makes sense. I don't think there's any overriding here, so it shouldn't be problematic how we order them
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 our case, there might not be any overriding leading to such weird behaviors, but in userland, it might still happen.
Don't we want to prevent that to happen? Remember custom site CSS is optimized too, so it might impact some users in the end 🤷♂️ And upgrading docusauurs means our own internal CSS also changes, which can lead to unexpected CSS ordering changes from one release to another (ie some beta.1 customCSS could be at the bottom, and with beta.2 this exact same custom CSS ends up being at the very top)
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 suggest you re-try with a class name with not completely overlapping rules (e.g. add an extra
content: "hey"
. As soon as the user is really attempting to override anything, the selector would go to the bottom. (But in fact in order for her to "override" anything she has to use the same selector as an existing one, instead of a new selector. Relative positions of distinct selectors don't matter; only their specificities do)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 is already the case with
test-marker-site-custom-css-unique-rule
, probably not worth adding another entry 🤷♂️I don't know, using
width: 100%
is a legit use-case for me, and the CSS selector of a user does not need to contain multiple rules, so it's eligible for de-duplication, even if it may not be the most common use-case🤷♂️ not really, there are many ways to create selectors targeting the same element, where those elements can have the exact same specificity, and insertion order still matters in those cases.
<div className="x y z"/>
You can target it with
.x
,.y
or.z
, all have the same specificity, and class insertion order will matter.If one of those classes gets optimized and put at the top for any reason (including custom site CSS), order might change in unexpected ways.
This also means that a user adding a custom rule might actually unintentionally trigger infima classes optimizations, leading to bugs in our theme
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.
Consider your example with
.ifm-full-width
and.ifm-item
containing the same rules. Does it matter if the user usesclassName="ifm-full-width"
orclassName="ifm-item"
?—No, they both lead to the same style being applied. Yes, Infima classes may get hoisted by a user class name, but if they are always perfectly identical in bahavior, why does it matter which one we use? As soon as the user's class has the slightest difference in bahavior it will be duplicated.I think we should trust the optimization algorithm, fix the site CSS insertion first, and after that try to come up with an actual counter-case to break the CSS
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.
agree, these are edge cases and less important than the general insertion order