fix: pin cascade-layer order and classify workspace symlinks#247
fix: pin cascade-layer order and classify workspace symlinks#247arbrandes merged 1 commit intoopenedx:mainfrom
Conversation
arbrandes
force-pushed
the
fix-layer-order
branch
2 times, most recently
from
5166a1e to
c9e5eed
Compare
arbrandes
force-pushed
the
fix-layer-order
branch
2 times, most recently
from
70761c6 to
71a508f
Compare
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
LGTM!
One naming nit and one out-of-scope comment.
| inject: true, | ||
| template: path.resolve(process.cwd(), 'public/index.html'), | ||
| chunks: ['app'], | ||
| chunks: ['layerOrder', 'app'], |
There was a problem hiding this comment.
nit: it might be nice to specifically mention stylesheets in the chunk name, so something like stylesheetLayerOrder or styleLayerOrder or cssLayerOrder
There was a problem hiding this comment.
Going with cssLayerOrder.
| const NODE_MODULES = /[\\/]node_modules[\\/]/; | ||
| export const PARAGON_PACKAGE = { name: '@openedx/paragon' }; | ||
| export const SHELL_PACKAGE = { name: '@openedx/frontend-base' }; | ||
| export const BRAND_PACKAGE = { name: /^@(open)?edx\/brand(-.+)?$/ }; |
There was a problem hiding this comment.
Not blocking, and out-of-scope for this PR, but the more I think about it the more I feel using a regex to match brand packages is a bit odd.
With the npm aliasing method used by MFEs, there were no restrictions on what site operators could name their brand packages.
I'm not sure how many site operators are using brand packages that don't match the regex, so it's possible this is a non-issue, but I figured it's worth flagging to at least think about.
There was a problem hiding this comment.
The alternative is to do what Atlas does, which is to require metadata in the package.json. I don't dislike that idea, not only for brand but for apps: that would give us a way to have an app layer literally just for apps, not as a catch-all. (And to put the node_modules stuff in its own layer before anything else).
But we can do this in a subsequent series of PRs.
CSS cascade-layer priority is locked in by the first mention of each layer name. With the order statement living at the top of `shell/style.scss`, other shell SCSS files reached earlier by webpack's import traversal were registering `shell` before `paragon` and inverting the intended order. Move the declaration to its own `shell/layer-order.scss`, loaded first via a dedicated webpack entry. Also classify named packages by package.json name, where possible. This makes it more flexible (so that it applies to workspaces, for instance). Fixes openedx#246. Co-Authored-By: Claude <noreply@anthropic.com>
arbrandes
force-pushed
the
fix-layer-order
branch
from
71a508f to
a0f1eb5
Compare
|
🎉 This PR is included in version 1.0.0-alpha.40 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Description
The
@layer paragon, shell, app, site, brand;declaration used to live at the top ofshell/style.scss. Because CSS cascade-layer priority is locked in by the first mention of each layer name, this only worked when that file was the first CSS the browser parsed; other shell SCSS reached earlier via webpack's import traversal was emitting@layer shell { ... }blocks first and inverting the intended order. Moved the declaration to its ownshell/layer-order.scss, registered as a dedicated webpack entry, and pinned its<link>to the top of<head>viaHtmlWebpackPlugin'schunksSortMode: 'manual'.Also fixed a related misclassification in npm-workspace / bind-mount dev setups. Paragon, shell, and brand rules now classify by the
namefield in each package'spackage.json(via webpack'sdescriptionData), which is independent of filesystem layout. The site/app split remains path-based, broadened to cover/packages/as well as/node_modules/.Fixes #246.
LLM usage notice
Built with assistance from Claude.