Admin UI: Add CSS files to sideEffects array#76609
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
153b16d to
9918887
Compare
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
I don't have much knowledge on these aspects of the repo. I will note that some packages apply the same side effects options, but, for example, Which makes me wonder what's the right move here. @youknowriad , @aduth, @tyxla or @jsnajdr maybe? |
We should fix the import '@wordpress/dataviews/build-style/style.css';might get eliminated by a bundler. |
CGastrell
left a comment
There was a problem hiding this comment.
LGTM! The fix works exactly as described.
|
Failing check doesn't seem related to this PR |
Yes, the e2e failures look like something that was broken in |
| "build-style/**", | ||
| "src/**/*.scss" |
There was a problem hiding this comment.
I'm confused by this containing both source files as well as the built artifacts, though I guess it stems from a general confusion about how styles are meant to be brought in with this package.
- If styles are meant to be imported automatically through importing JavaScript, I'd expect we'd only need
src/here and the/build-style/entrypoint should be removed - Vice-versa, if someone should be expected to import the stylesheet explicitly, I'd expect we'd only need
build-style/here and the.scssimport should be removed from the JavaScript code
|
I wonder if it's also wrong that we define gutenberg/packages/ui/package.json Line 44 in 4eeb1d5 |
|
To recap, it looks like there is some follow-up work to be done:
I don't have deep knowledge on the styles build pipeline, but at least this recap may help with future iterations. |
* add css files to sideEffects array * match other packages sideEffects Co-authored-by: dhasilva <thehenridev@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: CGastrell <cgastrell@git.wordpress.org> Co-authored-by: enejb <enej@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org>


What?
Closes #76606
Why?
To avoid CSS file imports from the admin-ui package being ignored.
How?
By adding an array to the
sideEffectsvalue containing CSS files.Testing Instructions
This one is tricky to test. I'm using https://github.com/dhasilva/minimal-wp-build-test/tree/test/admin-ui-side-effects to test this.
test/admin-ui-side-effectsbranchpnpm installpnpm run build, you should see the errors shown in Admin UI: CSS imports are ignored due to sideEffects set to false #76606package.jsonfiles of the admin-ui package to be the same as this PR. In my environment, they are located in:pnpm run buildagainTesting Instructions for Keyboard
Screenshots or screencast
Use of AI Tools
No AI tool used.