Skip to content
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

Revert removal of @wordpress/nux until WordPress-Develop stops consuming it #52444

Closed
peterwilsoncc opened this issue Jul 8, 2023 · 6 comments
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability npm Packages Related to npm packages [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Jul 8, 2023

Description

In #46110 the @wordpress/nux package was removed from the Gutenberg repository. As the WordPress-Develop is still consuming the package, this is causing significant issues while running npm run sync-gutenberg-packages for both the wp-6.2 and wp-6.3 distribution tags.

While the script is apparently successful, the incorrect versions of key packages are installed. @wordpress/components reverts to version 23.9.0; @wordpress/data reverts to 8.6.0.

Tagging @wordpress/nux@6.0.0 with the affected distribution tags has been attempted without success, see WP#57643 (comment).

Until WordPress-Develop (and thus WordPress releases) stop consuming the script, I think #46110 needs to be reverted in trunk and the affectedwp/* branches.

  • the failing sync script represents a significant hurdle for the security team doing backports. This increases the chance a mistake will be made during a security release
  • the order of operations needs to be to stop consuming packages before they are deleted.

If lerna can handle the directory structures, then it might be best to place the package out of the way, either in /packages-deprecated or /packages/deprecated-packages. It looks to be possible via the packages attribute in lerna.json.

cc @tellthemachines

Warning

The order of operations merging these pull requests will be important to ensure we end up with logical version numbers in each of the WP branches (ie, WP 6.2 runs a later version of nux that WP 6.1)

NPM currently lists @wordpress/nux as being version 6.0.0, although this doesn't appear to be used in any of the WordPress branches. It's possibly in a Gutenberg plugin release branch. To avoid version conflicts, I suggest lerna be run with custom version number bumps for the 6.2 branch

  1. Restore @wordpress/nux to WP 6.2 branch #52452 (version 7.0.0)
  2. Restore @wordpress/nux to WP 6.3 branch #52454 (if required, trunk may be backported to 6.3) (version 8.0.0)
  3. Restore @wordpress/nux to trunk #52455 (I think this will also be version 8 & backported in an ideal world)

As the release requirements are a little more complicated than usual, I suspect the workflow will not be able to select a custom version number. This will mean that someone with NPM publishing rights will need to run the commands manually. The current settings in the repository allow members of the gutenberg-core group to push to branches.

Step-by-step reproduction instructions

  1. Checkout commit WordPress/wordpress-develop@ab9d5e3 (this commit is prior to a package update)
  2. Run npm run sync-gutenberg-packages -- --dist-tag=wp-6.3
  3. In the wp-packages:refresh-deps phase of the script, several scripts will be downgraded:
    The following dependencies are outdated: 
    [
      {
        name: '@wordpress/components',
        required: '23.9.0',
        actual: '25.1.3'
      },
      { name: '@wordpress/data', required: '8.6.0', actual: '9.5.1' },
      {
        name: '@wordpress/private-apis',
        required: '0.14.0',
        actual: '0.17.1'
      },
      { name: 'memize', required: '1.1.0', actual: '2.1.0' }
    ]
    

Screenshots, screen recording, code snippet

git diff package.json after running script at WordPress/wordpress-develop@ab9d5e3

diff --git a/package.json b/package.json
index 34ab5f7b1c..2c6c179a0f 100644
--- a/package.json
+++ b/package.json
@@ -90,12 +90,12 @@
 		"@wordpress/block-serialization-default-parser": "4.35.1",
 		"@wordpress/blocks": "12.12.1",
 		"@wordpress/commands": "0.6.3",
-		"@wordpress/components": "25.1.3",
+		"@wordpress/components": "23.9.0",
 		"@wordpress/compose": "6.12.1",
 		"@wordpress/core-commands": "0.4.3",
 		"@wordpress/core-data": "6.12.3",
 		"@wordpress/customize-widgets": "4.12.5",
-		"@wordpress/data": "9.5.1",
+		"@wordpress/data": "8.6.0",
 		"@wordpress/data-controls": "3.4.1",
 		"@wordpress/date": "4.35.1",
 		"@wordpress/deprecated": "3.35.1",
@@ -125,7 +125,7 @@
 		"@wordpress/preferences-persistence": "1.27.1",
 		"@wordpress/primitives": "3.33.1",
 		"@wordpress/priority-queue": "2.35.1",
-		"@wordpress/private-apis": "0.17.1",
+		"@wordpress/private-apis": "0.14.0",
 		"@wordpress/redux-routine": "4.35.1",
 		"@wordpress/reusable-blocks": "4.12.3",
 		"@wordpress/rich-text": "6.12.1",
@@ -155,7 +155,7 @@
 		"json2php": "^0.0.7",
 		"lodash": "4.17.21",
 		"masonry-layout": "4.2.2",
-		"memize": "2.1.0",
+		"memize": "1.1.0",
 		"moment": "2.29.4",
 		"objectFitPolyfill": "2.3.5",
 		"path-to-regexp": "6.2.1",

Environment info

  • npm 6.14.17
  • node 14.21.2
  • MacOs 12.6.7

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@peterwilsoncc
Copy link
Contributor Author

Process

Based on my knowledge of lerna and versioning of packages developed in the Gutenberg repository, I think the process for this is going to require a fairly careful order of operations:

When tagging the version in the 6.2 branch, we'll be able to rely on Lerna to get the version correct as the tagging history will be in the branch.

For the 6.3 and trunk version bumps, a custom version number will need to be specified to ensure the major version numbers are higher than 6.3's version number (6.3 and trunk won't have the new tag from 6.2 in their history).

Lerna does not allow skipping the versioning of altered packages, so any changes to other packages will need to have custom versions specified to be minor bumps only. I suspect only nux will need to be published as other packages won't change and will have been bumped for the most recent WP security releases.

@jordesign jordesign added [Type] Task Issues or PRs that have been broken down into an individual action to take npm Packages Related to npm packages labels Jul 10, 2023
@peterwilsoncc
Copy link
Contributor Author

Pinging @WordPress/gutenberg-core on this.

The tl;dr is that the npm run sync-gutenberg-packages script in WP-Dev downgrades a couple of key packages (data and components) as the installed version of nux lists them as dependencies. See the original description for version details.

I'm concerned something is going to be missed during a security release and result in incompatible packages been released if we don't resolve the issue for each branch, thus I've created three PRs:

My secondary concern is that the documented process for updating packages doesn't work, which adds extra overhead for committing contributors during a release. So far it's been caught each time.

As we're going to need to specify the version numbers for nux manually, the lerna commands will need to be run manually via the command line (suggestions to avoid this are most welcome). My understanding is that these are the commands that will need to be run on each branch:

nvm use
npm ci
npm run prebuild:packages
npm run build:packages
npx lerna publish --dist-tag wp-6.2 --no-private # dist-tag changes per branch

Once restored, we can try to move the deprecated package out of the way in the lerna workspace but for the 6.3 release simpler is better.

@ramonjd ramonjd added the Backwards Compatibility Issues or PRs that impact backwards compatability label Jul 16, 2023
@ramonjd
Copy link
Member

ramonjd commented Jul 16, 2023

I don't have much of a nuanced opinion on this, so happy if it doesn't count for much.

I understand this proposal to be, in essence, about maintaining backwards compatibility with Core.

Given the lengths we go to achieve backwards compatibility in every other area of WordPress, I don't see any controversy in reinstating these packages until they're fully deprecated from Core.

@peterwilsoncc
Copy link
Contributor Author

I understand this proposal to be, in essence, about maintaining backwards compatibility with Core.

For the time being, my main priority is to get the package update script working in WP-Dev again. Partly to future proof security updates so the security team don't accidentally downgrade packages during the very hectic release process.

In a WP-Dev PR @azaozz suggested nulling the scripts out as a possible approach in the future WordPress/wordpress-develop#3775 (comment) to avoid JavaScript fatals, but that's for another ticket -- which I'll open once this is resolved.

@peterwilsoncc
Copy link
Contributor Author

Having restored to the wp/6.2 and wp/6.3 branches, I can confirm the npm run sync-gutenberg-packages -- --dist-tag=wp-x.x scripts are now working as expected on WP-Dev's trunk and 6.2 branches.

Leaving this open until the PR for GB trunk is merged.

@gziolo
Copy link
Member

gziolo commented Aug 4, 2023

I think we can close it now that #52455 got merged.

@gziolo gziolo closed this as completed Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability npm Packages Related to npm packages [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

4 participants