fix(core): correctly order CSS imports #5987
Conversation
Josh-Cena
added
pr: bug fix
|
✔️ [V2] 🔨 Explore the source changes: 2297f8d 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/619b27f4af138c0008bf388c 😎 Browse the preview: https://deploy-preview-5987--docusaurus-2.netlify.app |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5987--docusaurus-2.netlify.app/ |
|
Size Change: -65 B (0%) Total Size: 892 kB
ℹ️ View Unchanged
|
This comment has been minimized.
This comment has been minimized.
|
After re-reading that thread it seems #3678 is not solved... That one was hoping to make custom CSS, currently loaded via client modules, override module CSS styles |
| import React from 'react'; | ||
| // Load this at the very top! | ||
| // client modules have to be loaded before anything else... | ||
| import './client-lifecycles-dispatcher'; |
There was a problem hiding this comment.
I actually have no idea we import client-lifecycles-dispatcher instead of @generated/clientModules here. But side-effects are too hard to reason so I didn't refactor it
There was a problem hiding this comment.
I think you are right and we should do this change, it was probably historical.
But yeah, hard to reason about and see possible side-effects.
|
I'd rather not do this kind of change now until we have defined properly what kind of order we want exactly, have better test on CSS ordering issues, and have a concrete plan to solve them once for all. This kind of change can produce annoying breaking changes for users that may need to rewrite some of their site's CSS. We don't have a lot of custom site CSS on Docusaurus, and need to make sure such change is fine for sites like ReactNative.dev. If we really need to do such a breaking change, I'd rather do only one instead of doing multiple attempts, each producing a breaking change. IMHO the order should rather be:
|
This would not affect most sites because it's making us actually abide to the spec we made. We have to make sure that styles are applied in the order of
This is exactly the ordering we are supposed to keep. But if you look at the
I'll soon make another PR to handle that. Do you think we should apply that here as well? Note that we are just restoring the pre-#3104 behavior. Had #3104 not been merged, it doesn't really matter how we arranged the imports because they didn't have side-effects. We should find a tool to mark import side-effects, not sure if it would be easy |
|
The RN site is using https://github.com/facebook/react-native-website/blob/main/website/docusaurus.config.js#L73 theme: {
customCss: [
require.resolve('./src/css/customTheme.scss'),
require.resolve('./src/css/index.scss'),
require.resolve('./src/css/showcase.scss'),
require.resolve('./src/css/versions.scss'),
],
},
Unless we have a test plan for it, this is just a random guess 🤪 Fixing this issue with a few lines of code is probably not the time-consuming part, it's actually having a test plan and executing it that is the time-consuming part. I'd like such a test to be automated so that we never regress again on CSS ordering issues.
No, this has to be handled atomically in a single PR atomically, not split into multiple PRs. We define an order once for all, test it automatically on our Docusaurus site, test it in a few third-party sites to ensure this order is correct, and then we stick to it forever. I can't dedicate much time to all this atm as I'm still trying to focus on the category index feature. If you want to work on this, be ready to invest much more time in it because it's unlikely I'll want to merge a PR like this one without a strong understanding of the possible annoying side effects for our user base. We need to limit the number of breaking changes. If that helps, I can see how to produce a canary release for that branch and you can open PRs to some third party sites so that we can see what kind of impact it has |
|
Sure, will see what we can do to test this PR 👍 I would figure out about making the |
Josh-Cena
changed the title
Josh-Cena
added
the
status: don't merge yet
|
@slorber I understand your concern, but I still want to get the fix somewhat done correctly. What about this: Instead of putting the import at the very top and risk reordering all CSS: + import './client-lifecycles-dispatcher';
import React from 'react';
import routes from '@generated/routes';
import renderRoutes from './exports/renderRoutes';
import {BrowserContextProvider} from './exports/browserContext';
import {DocusaurusContextProvider} from './exports/docusaurusContext';
import ErrorBoundary from '@docusaurus/ErrorBoundary';
import PendingNavigation from './PendingNavigation';
import BaseUrlIssueBanner from './baseUrlIssueBanner/BaseUrlIssueBanner';
import Root from '@theme/Root';
import Error from '@theme/Error';
- import './client-lifecycles-dispatcher';We pretend that #3104 was actually done by putting all imports last: import React from 'react';
import routes from '@generated/routes';
import renderRoutes from './exports/renderRoutes';
import {BrowserContextProvider} from './exports/browserContext';
import {DocusaurusContextProvider} from './exports/docusaurusContext';
import PendingNavigation from './PendingNavigation';
import BaseUrlIssueBanner from './baseUrlIssueBanner/BaseUrlIssueBanner';
import Root from '@theme/Root';
import './client-lifecycles-dispatcher';
+ import ErrorBoundary from '@docusaurus/ErrorBoundary';
+ import Error from '@theme/Error';We leave all the other imports untouched. This would still fix the issues, and had #3104 not landed, the behavior would be identical in every way to the current behavior. Does that look sensible as a better temporary fix than just removing the app-level error boundary? Now that we do know what's wrong, I feel we deserve a better fix. I would send another PR and you can merge that one if you think the fix described above makes sense. We would keep this one to actually fix the issue from its root. |
|
I'm fine with the current change of imports order. After all, this will lead back to the previous of CSS ordering, where infima/admonitions styles came first, then custom CSS files, and finally built-in Docusaurus components styles (which currently cannot be simply overridden without using To reproduce #3678, try overriding built-in |
I understand Sebastien's concern. While he's probably being more speculative than we are, the moment we assert there's no breaking change with the addition of in my understanding / I believe it means we can't be sure yet, and that can be very problematic. In practice, although we are probably moving ourselves towards the correct side, we never know what kinds of convoluted setups there are in the wild. Maybe they relied on some (terrible) hacks that only work with our previous import order and this change breaks their website. Mandatory XKCD for the day though, for this kind of fear: I do assert that either fix is safe, no matter where we put the client modules import, as long as it stays above the error boundary import, because these are the only two imports with side-effects. |
There was a problem hiding this comment.
That change seems reasonable to me.
We already have a stylesheets API to easily override global CSS. I guess CSS is added after infima + CSS modules? https://docusaurus.io/docs/api/docusaurus-config#stylesheets
Also, should we keep theme.customCss, deprecate it, or change its behavior? with this change, customCss CSS will be loaded earlier than before (before theme CSS modules), which means that some unexpected CSS might become applied to user sites while it was not the case before 🤷♂️ . This may not impact us, but it may impact other highly customized websites like React-Native.
And yes, all this is Hyrum's law (https://www.hyrumslaw.com/) in practice: current users depend on our legacy wrong behavior. Some users may suffer from this change and we should aim to reduce their pain, by ensuring a well-defined order once for all with automated tests.
What I suggest:
PR 1:
- Revert the error boundary in core to ensure we revert the CSS order issues to the former state (temporary)
- Dogfood usage of
stylesheetson our own website andtheme.customCSSon our own website, eventually add things like "marker CSS classes" that we can easily monitor like.test-css-module {}or.test-stylesheets {}or.test-custom-css {} - Monitor CSS diff changes in our own website (inspired by refactor: replace dist in git by diff comment facebookincubator/infima#152?)
- Eventually add an automated test to ensure marker CSS classes are in a correct order
PR 2:
- Do this client modules import change
- ensure that the monitored diff looks expected in terms of order, particularly that the marker CSS classes are in the correct order.
- Publish canary from PR
- Test on RN/Jest websites and see how painful it is for them to fix issues, create PRs that can help the community to migrate their CSS files
| import React from 'react'; | ||
| // Load this at the very top! | ||
| // client modules have to be loaded before anything else... | ||
| import './client-lifecycles-dispatcher'; |
There was a problem hiding this comment.
I think you are right and we should do this change, it was probably historical.
But yeah, hard to reason about and see possible side-effects.
Josh-Cena
removed
the
status: don't merge yet
|
Closing in favor of #6227 |
Motivation
Fix the problem patched in #6052. Fix #3678.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
TODO