-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update npm packages for 6.4.2 #5665
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM and everything works as expected in testing (I followed the testing steps for the included PRs).
One thing I'm slightly puzzled about is why all the packages are getting version bumps. Did anything change in the workflow from last release to this one? I notice this has been happening for all the updates this release, whereas e.g. for 6.3.2 only the changed packages got bumped. This is not blocking by any means 😅, just wondering about it.
Noting that the following changes were also included in this PR: Add check for lightbox values during image block migration Add context for translators to any unclear usage of "synced" Patterns: use existing download function for JSON downloads to fix no… This is as expected because they were all marked for inclusion in the next minor release. |
Committed in r57109. Leaving this open for commit to the release branch though not sure if it's best to point this PR at the release branch instead of trunk or just open a new one (we should in any case check CI passes with this diff against the release branch) |
Thanks so much for helping with this and testing!
This has been mentioned before during the 6.4 cycle but I can't find where right now. I vaguely remember it being mentioned and it was fine for now, but going into 6.5 maybe we should look more closely at this. 😅 I don't know if there's been an update to the publishing actions or if Lerna has been updated, but it's almost as if Lerna has no context for the current changes, so it assumes everything needs a version bump. I'm not sure what's changed between 6.3 and 6.4 though - maybe @gziolo knows more? |
After checking the publishing job I can share that I identified the reason why we still see all packages published. I landed some time ago the fix in the Gutenberg repository that potentially addresses the issue encountered with WordPress/gutenberg#55334 but it only impacts
It worked smoothly with WordPress 6.3 and lower. However, once Node.js and npm upgrades happened, we had to stop using the Node script and move all logic to the GitHub workflow to support all the different npm versions used with older wp branches. This is why workflow requires |
Ah this makes sense, thanks @gziolo! I hadn't seen WordPress/gutenberg#55334.
@SiobhyB and I discussed this early in the 6.4 cycle and we decided to go with publishing the packages targeting the
The docs screenshot does show the target as
I'm happy to help handle this. |
Oh I see, thanks @gziolo !
I agree with @mikachan that the docs could be more explicit about which branch to trigger the workflow from, as I also found that super confusing in the past 😅 Cherry picking the fix to the release branch would also be good to avoid confusing anyone packaging a minor release from this branch in the future. |
I cherry-picked WordPress/gutenberg@1dd3145 in the
Let's do it as well, as it might prevent similar issues in the future. I understand how the combo box for selecting the branch might confuse people. The intention GitHub has with it is to allow the selection of the branch from which the GitHub workflow file is going to be read on CI. In effect, when selecting the https://github.com/WordPress/gutenberg/blob/wp/6.4/.github/workflows/publish-npm-packages.yml |
@tellthemachines, you mentioned above that we may either need to point this PR at the release branch (I guess this is Also, we have one more change that needs to be included in the package update. I'm in the process of updating the packages again here - should I create a new PR for this, or include it in this PR? I know this PR has already been committed to trunk, so I'm guessing it's best to open a new PR. |
@mikachan in that case it's probably easier to open a new PR for trunk with that extra change, and then assuming it's all good, open a third PR for the release branch including all the changes. |
Will do, thank you! |
Here's the new PR for trunk with the single extra change: #5696 The tests seem good, although it looks like there's an error with the Playground build process here. I'm not sure if that's a blocker or not. I went ahead and created a PR against the release branch too with all the changes: #5698. All the tests passed here. Thanks so much for your help 🙇 |
I'm going to close this PR as these changes have already been committed to trunk in r57109. We have multiple PRs open for the npm package update (including a separate PR for the release branch), so trying to reduce any confusion! |
Trac ticket: https://core.trac.wordpress.org/ticket/59828
Included changes:
New published packages for this update: WordPress/gutenberg@70b30e9
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.