Renovate: update bundled wp package rules#50007
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
anomiex
left a comment
There was a problem hiding this comment.
I like the general idea. Some comments inline.
I don't think this change would result in #49272 getting closed. Since the groupName isn't changing, it should still recognize it as the same change. I'll test that in a fork in a bit.
| // @wordpress/stylelint-config is grouped with theme because its WPDS rules lint against | ||
| // the token set shipped in @wordpress/theme — the two should be updated in lockstep. |
There was a problem hiding this comment.
Is this true? Looks like the actual rules are in @wordpress/theme, and @wordpress/stylelint-config just loads them from there.
There was a problem hiding this comment.
Hmm I'm not sure — I think that would work if @wordpress/stylelint-config would define @wordpress/theme as peer dependency, but it's a direct dependency so it likely uses its own version?
"dependencies": {
"@stylistic/stylelint-plugin": "^3.0.1",
"@wordpress/theme": "^0.16.0",
"stylelint-config-recommended": "^14.0.1",
"stylelint-config-recommended-scss": "^14.1.0"
},
I had exactly this mismatch happen after updating wp-theme+variables in JP locally to new ones, and stylelint started complaining it until I updated it. So I assumed stylelint was the issue but I did not confirm that. If you think it's safe to leave out, happy to do so.
There was a problem hiding this comment.
Hmm. Best thing in that case might be to pnpmfile-hack it to turn that into a peer dep. Or to >=0.16.0. 😀
Otherwise, I think it'd work fine for updating @wordpress/theme to something like 0.16.1, pnpm dedupe should take care of the indirect dep, but not 0.17.0 (because for 0.x caret deps like ^0.16.0, it works like >=0.16.0 <0.17.0).
There was a problem hiding this comment.
It would pick the right version if @wordpress/stylelint-config defined @wordpress/theme as a peerDependency, right? Just then we would need to rely on consumers to always include the dependency, which would not be ideal. Is there another way to improve this upstream?
| // @wordpress/build is still heavily developed; its dashboards share the same bundled | ||
| // packages and test surface, so exercising build alongside the rest is practical. | ||
| // | ||
| // Bundled @wordpress/grid is missing here as its updates with "Widgets Dashboard" grouping separately. |
There was a problem hiding this comment.
Maybe we should just merge that here too?
There was a problem hiding this comment.
My preference as well, and likely safer long-term, although right now anything using grid/dashboard stuff is non-production for at least another month.
@Automattic/red @Automattic/ventures WDYT if we merge the Dashboard renovate block added in #49815 with the Bundled WordPress deps? Might be more straightforward and safer with cross-dependencies.
jetpack/.github/renovate.json5
Lines 150 to 163 in b739800
There was a problem hiding this comment.
Checked on Slack, internal ref p1782814626414479-slack-CK365S85V
There was a problem hiding this comment.
Thanks for tagging us, and sorry for the late reply!
The main reason for adding it into a separate bundle, was similar to that of @wordpress/build, where it is still heavily being developed and not heavily used in production environments ( not entirely true for wp-build anymore ).
Plus of-course the fact that Ventures & Red are the only one's using the dashboard related packages atm. There is a high chance that we will also be updating the packages more frequently, because of the nature of active development.
Having said that, if the other teams in the group don't mind also being pinged on updates of these packages, then I am more then happy if we move it over, and don't have a strong sense against that.
although right now anything using grid/dashboard stuff is non-production for at least another month.
I did almost say we wait, until it is production and then merge the groups.
There was a problem hiding this comment.
I think if that's the only reason, it'll be fine to merge now. I'd expect teams to mostly review what they’re familiar with, so others wouldn’t look into dashboard stuff anyway.
It still doesn’t stop you from updating dashboard deps independently e.g. to next- versions outside regular update cycles if you need to.
The upside is also that ui and theme will be kept in sync with dashboard packages, which is good since there are inter-dependencies between those packages anyway.
Worked fine in my fork on anomiex#58, no deletion. |
anomiex
left a comment
There was a problem hiding this comment.
Two open questions, I think:
- Merge the widget packages into this group too?
- Pending reply from the teams using those.
- Add stylelint-config to the group, or use pnpmfile to just make sure it picks up the shared version of the theme package?
- Personally, I'm inclined towards the latter. If others agree, I'll volunteer to make the change.
081cd32 to
a12a57c
Compare
Feel free to push to this PR! I've just rebased PR with the latest trunk. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
e6e20c7 to
d5311e8
Compare
|
Merged dashboard section into bundled packages + rebased. Ready for final look (feel free to merge!). |
| // Make sure @wordpress/stylelint-config gets whatever @wordpress/theme is installed. | ||
| if ( | ||
| pkg.name === '@wordpress/stylelint-config' && | ||
| pkg.dependencies[ '@wordpress/theme' ]?.startsWith( '^' ) |
There was a problem hiding this comment.
Non-blocking nit: pkg.dependencies[ '@wordpress/theme' ]?.startsWith guards the value but not pkg.dependencies itself — inconsistent with the pkg.dependencies?.[ … ] style in the stylelint-config block just above. Harmless in practice (stylelint-config always has dependencies).
There was a problem hiding this comment.
I think I'd actually prefer it breaks loudly on bumping up the version if it stops having deps or theme dep specifically, which I think is better and we would notice to clean this up. :-)
Follow-up to #49272 (review)
Latest bundled WP packages update revealed a few gaps.
Best to have #49272 merged first before this so that Renovate doesn't close it.We can merge this before the bundle update.Proposed changes
@wordpress/themeto avoid mismatching versions of bundled deps (notably@wordpress/ui,@wordpress/dataviews,@wordpress/admin-uiand others) using style tokens from different version than Jetpack is shipping.@wordpress/stylelint-configto lint invalid design tokens, we need to bump that together with@wordpress/theme.@wordpress/style-runtimefrom the latest bundled deps listRelated product discussion/links
Does this pull request change what data or activity we track or use?
Testing instructions