Update stylelint to v17, including related packages - #80738
Conversation
…nns/gutenberg into feature/update-stylelint
|
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. |
|
I haven't looked deeply, but you might try adding gutenberg/test/unit/jest.config.js Lines 82 to 85 in 915af00 Jest doesn't actually process ESM, and this pattern is a way to tell Jest to transpile (to CommonJS) some of our dependencies that need it. Since Stylelint went ESM-only in v17, it likely needs it. |
manzoorwanijk
left a comment
There was a problem hiding this comment.
Looks fine to me. Let us fix the changelog though.
Co-authored-by: Manzoor Wani <manzoorwani.jk@gmail.com>
|
@manzoorwanijk I've updated the changelog with your suggestion. Just for avoidance of doubt, I think changelogs for the components and UI packages are unnecessary as the only changes are either indentation or updates to stylelint disables, neither of which have any change on the styling rules at all. The theme package is slightly different, the only change is the update of a dev dependency, however because there was no change to any other code and the peer dependency remained the same, I don't believe this warrants a changelog entry either. |
|
I don't think adding |
stylelint 17 dropped its CommonJS build, so it can no longer be imported from Jest's CommonJS runtime. Move the lint into a small ESM runner script spawned as a child process, matching how the stylelint-config tests already invoke stylelint. Because a plugin instance can't cross a process boundary, the tests now reference the plugin under test by path instead of importing it.
|
@aduth @ciampo, should we try the It's getting messy. I wish we get rid of Jest in favor of Vitest. |
If it works, sure. I'd be happy with anything that gets us closer to true ESM. I just assumed it would break a bunch of stuff if we enabled it. |
Yes, it breaks lot of other tests. Let us stick with this solution for now. |
|
@manzoorwanijk Do you think this pull request needs anything else? For your comment at #80738 (comment), my understanding is that the ignore list is what allows Jest to be able to import it by transpiling to CommonJS. The regular expression with lookaheads is confusing, but my read of it is: "transpile everything to CommonJS, but not node_modules as they should be loaded as-is, with the exception of these packages that should also be transpiled to CommonJS". We should probably rebase/merge the latest |
Let's make it happen! I spent some time researching and coming up with a plan:
|
No, it's ready, but TBH, I am not happy with the mess this ESM/CJS fight creates here.
The reason is that merely adding stylint there won't help as it dies on some transitive deps like |
Now you can see the CSS errors in CI after trunk merge. |
|
Ah yeah, that's annoying, I specifically made sure trunk was up to date when I started to avoid this. Shall we add these to the suppressions for now so it isn't a blocker for this PR? |
I don't think suppressions is a good idea, and yes, I do think it's a blocker for this PR. Let us create a preparatory PR that fixes those lints in trunk and then we can update this PR. |
|
@manzoorwanijk I've synced the latest version of trunk to my fork, run npm install and then npm run lint:css. I'm not getting any errors against trunk. Do you think this is accurate? Do we just have to make the changes even though they're not flagging yet? |
That is expected because those lints are due to the version upgrade here.
It's fine. Let us fix those lints here in this PR. |
|
Okay, I have done the work to fix those linting issues. All the changes should be transparent in normal cases, however, it's possible these changes may have an impact on sites which have overridden those styles, especially on the blocks front end styling. Note I have not done any testing or changelogs for these changes, and the safer method in this case may be to revert my commit and add "stylelint-disable-next-line" for all the violations instead, as this won't change styling but will still apply rules to the new code being added. I have run out of time on my end for this week, but I may be able to look at these issues next weekend and do some more in-depth testing if no-one else can do this in my absence. |
|
Thanks for the work here @mikeybinns ! At a first glance a lot of these look like valid improvements for legacy CSS properties, for example. I'll take a closer look to understand the backwards-compatibility ramifications. You raise an interesting point about the potential downstream impact, though (a) I don't know that it's reasonable to expect that we can't ever make CSS changes to a block (i.e. not a public API contract) and (b) many of these being legacy CSS properties with equivalent overrides, the updated property would be the preferable extension point. I'd be curious what might happen if a theme overrides |
| display: inline-block; | ||
| text-align: center; | ||
| word-break: break-word; // overflow-wrap doesn't work well if a link is wrapped in the div, so use word-break here. | ||
| overflow-wrap: break-word; |
There was a problem hiding this comment.
Some of these specifically call out issues in the alternative that we switch to. And if still valid, then we may want to keep it and disable the rule for the instance instead.
That might also extend to all of the places we're removing word-break and overflow-wrap if it's not functionally equivalent. To your comment in #80738 (comment), we may want to exempt all of these instances, or even consider disabling the rule altogether.
At the very least it wouldn't be the same value. MDN's documentation for word-break: break-word specifically mentions as having 'the same effect as overflow-wrap: anywhere combined with word-break: normal" so I don't think overflow-wrap: break-word; is the equivalent.
There was a problem hiding this comment.
I did try out in a codepen whether a div > a structure with overflow-wrap: break-word and word-wrap: break-word and I couldn't see a difference, but yeah, more testing should be done to confirm this.


What?
Closes: #75047
Closes: #75503
Why?
There was no change or response from the original PR creator on branch #75503 and that branch only updates the stylelint package in one place, it didn't account for updating the package in other places in the repo or updating any breaking changes.
How?
I have:
Testing Instructions
Use of AI Tools
I have not used AI in any capacity for this migration.