Make WordPress Core

Opened 6 months ago

Last modified 5 days ago

#62839 reopened defect (bug)

sync-gutenberg-packages script is not respecting .npmrc file

Reported by: desrosj's profile desrosj Owned by:
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-test-info changes-requested
Focuses: Cc:

Description

It seems that running npm sync-gutenberg-packages results in some packages being duplicated within the package-lock.json file despite prefer-dedupe being set to true in .npmrc.

To test, run the following steps and committing after each one to a test branch, or staging changes so further changes are trackable:

  • run npm dedupe on trunk to properly dedupe.
  • run npm sync-gutenberg-packages.
  • run npm dedupe.

There is also the following error when running sync-gutenberg-packages:

wordpress-develop/tools/release/sync-gutenberg-packages.js:110
const versionConflicts = Object.entries( packageLock.dependencies )
TypeError: Cannot convert undefined or null to object

This may be related to ticket:62190#comment:17 from @kevin940726.

Change History (12)

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


6 months ago

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


5 months ago

This ticket was mentioned in PR #8431 on WordPress/wordpress-develop by jtquip88.


4 months ago
#3

  • Keywords has-patch added; needs-patch removed

npm install in sync-gutenberg-packages script is nto respecting settings specified in .npmrc file. To troubleshoot, I ran npm config list from inside the sync-gutenberg-packages script and observed the following output.

; engine-strict = true ; overridden by env
; legacy-peer-deps = true ; overridden by env
; lockfile-version = "3" ; overridden by env
; prefer-dedupe = true ; overridden by env
; save-exact = true ; overridden by env
; save-prefix = "" ; overridden by env

This indicates .npmrc settings are being overridden by environment variables. This means the beahvior might differ based o where script is ran. Made a change to explicitly use local .npmrc file while issuing npm install command.

Trac ticket: [](https://core.trac.wordpress.org/ticket/62839)

#4 @desrosj
3 months ago

  • Keywords needs-testing added
  • Milestone changed from 6.8 to 6.9

Unfortunately someone did not get the time to review this leading up to 6.8.

#5 @SirLouen
3 months ago

  • Keywords needs-testing-info reporter-feedback added; needs-testing removed

Combined Issue Reproduction and Test Report

Description

🟠 This report can't validate much about this whole issue

Patch tested: https://github.com/WordPress/wordpress-develop/pull/8431.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.4
  • Database: mysqli (Server: 8.0.41 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 134.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.1
  • Plugins:
    • Test Reports 1.2.0

Reproduction Steps

  1. Run npm dedupe --verbose (without verbosity, you cannot see if it gets stuck at some point), I can't see any changes after this.
  2. ❌ Run npm run sync-gutenberg-packages. File changes, but I can't confirm any duplicates.
  3. 🐞 Can confirm the TypeError during sync-gutenberg-packages
  4. ❌ After each dedupe I can't confirm any new changes in package-lock.json. I can't confirm any duplicates either.

Expected

  • Given the absence of duplicates, I'm not 100% sure what to expect
  • The error is gone

Actual Results with the Patch

  1. ✅ The error is gone with the patch.
  2. 🟠 No real impact done by this patch, I can't see a single difference between package-lock.json before and after the patch.

Additional Notes

According to the patch creator:

This is due to the fact that in generated package-lock.json file, dependencies are not a top level property/field, rather it is nested inside packages i.e each package. As a result, packageLock.dependencies is undefined. This needs some more investigation so I am hoping to raise a separate PR for that

Given that I can't reproduce the original issue (duplication of packages), I would suggest that @desrosj does a double check to see what is going on. Also, it would be great to get additional information and thoughts from jtquip88, as I'm not really sure how is affecting both the error and the patched version.

I think that if we just wanted to wipe the error, something as minimal as this:

const versionConflicts = Object.entries(packageLock.dependencies || {});

Would have done the trick as well.

PS: It's a pain to test this, it takes one year to run all the processes.

Supplemental Artifacts

This is the full error:

TypeError: Cannot convert undefined or null to object
at Function.entries (<anonymous>)
at getMismatchedNonWordPressDependencies (/home/admin/src/wordpress/wordpress-develop/tools/release/sync-gutenberg-packages.js:110:34)
at main (/home/admin/src/wordpress/wordpress-develop/tools/release/sync-gutenberg-packages.js:36:28)
at Object.<anonymous> (/home/admin/src/wordpress/wordpress-develop/tools/release/sync-gutenberg-packages.js:225:1)
at Module._compile (node:internal/modules/cjs/loader:1469:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
at Module.load (node:internal/modules/cjs/loader:1288:32)
at Module._load (node:internal/modules/cjs/loader:1104:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
at node:internal/main/run_main_module:28:49

@SirLouen commented on PR #8431:


3 months ago
#6

@jtquip88 please check this

#7 follow-up: @jtquip88
3 months ago

@SirLouen Thanks for testing this.
Since there were no duplicates to begin with, it's expected that this patch wouldn’t cause any differences. The patch doesn’t materially change how we fetch and structure dependencies—it simply ensures that local .npmrc settings are honored. This means that if there were duplicates, they would now be deduplicated due to the prefer-dedupe=true setting in .npmrc.

Regarding this error:

TypeError: Cannot convert undefined or null to object:

I didn’t include a fix for this because I assumed we’d want to investigate and address it separately. This error occurs because the parsing logic expects "dependencies" to be a top-level property in package-lock.json. However, in newer versions of npm (v7+), dependencies are nested under individual packages within the "packages" section.

Until npm v6, the "dependencies" field was indeed a top-level property. The parsing logic was likely written when npm v6 was being used so this means the issue has existed for a while but remained unnoticed.

Next steps:
If we still want to keep the version conflict resolution logic, we could rewrite it to support the new structure in package-lock.json. I’m happy to take that on—just wanted to align with the team here first If we want to fix it or just suppress the error.
cc @desrosj

Last edited 3 months ago by jtquip88 (previous) (diff)

#8 in reply to: ↑ 7 @SirLouen
2 months ago

  • Keywords needs-test-info close added; needs-testing-info removed
  • Resolution set to invalid
  • Status changed from new to closed

I've provided some testing instructions just in case anyone else could replicate this issue that @desrosj is reporting. I couldn't be on my side, and we are assuming there are no duplicates to begin with.

Replying to jtquip88:

I didn’t include a fix for this because I assumed we’d want to investigate and address it separately. This error occurs because the parsing logic expects "dependencies" to be a top-level property in package-lock.json. However, in newer versions of npm (v7+), dependencies are nested under individual packages within the "packages" section.

In this specific regard, I agree with you, and I would suggest you opening a new ticket with the proposed changes to reanimate the conversation around this topic and if possible adding some testing instructions to replicate.

I'm going to mark this post as a close candidate with a potential worksforme.

#9 @SirLouen
2 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

I accidentally closed. Reopening

#10 @desrosj
5 days ago

  • Keywords has-test-info added; reporter-feedback needs-test-info close removed

Thanks for the PR @jtquip88.

The PR looks like it should fix the issue. But I'm having trouble confirming because I'm still seeing duplicates.

For anyone looking to test, this expands on my originally listed steps and is probably the best process to follow outside of actually using the script to apply updates:

  • Create a branch. This can be from trunk or a recent version like 6.7 (I don't think 6.8 will have any updates currently).
  • Run npm dedupe (--verbose will give you more detail, but is not required).
  • Commit any changes that result. You now have a clean, deduped lock file.
  • Run npm run sync-gutenberg-packages. If you used a branch (say 6.7), then you will need to include -- --dist-tag=wp-<VERSION> where VERSION represents the major branch you are attempting to update (more documentation on the script itself can be found in the block editor handbook).
  • Run npm run post-sync-gutenberg-packages.
  • Commit any changes to your branch. You now have an updated branch containing the latest changes for that WP major version.
  • Run npm dedupe again.

Before the patch, this should result in new changes to the package lock file. This demonstrates that the prefer-dedupe option in the .npmrc file is not respected.

I am currently unable to reproduce the issue when using trunk to test without the PR, even though there are package updates that are applied.

However, I was able to reproduce when using the 6.7 branch and npm run sync-gutenberg-packages -- --dist-tag=wp-6.8 (note updating to the next major branch) and the PR did not fix the problem when rerunning.

A note about what duplicates means in this context to clarify. Running npm dedupe attempts to reduce duplication within the package tree stored within the package-lock.json file. So it's not that packages will be listed twice in package.json, but rather once (or as few times as possible) within package-lock.json. These changes will be really hard to notice on manual review because the lock file is over 37K lines. The best way to notice any changes would be running git diff.

#11 @jtquip88
5 days ago

Thanks for the detailed testing steps @desrosj. I am going to try to test this again with 6.7 branch and update here.

#12 @SirLouen
5 days ago

  • Keywords changes-requested added

Reproduction Report

Description

✅ This report validates that the issue can be reproduced.

Environment

  • WordPress: 6.7-src
  • PHP: 8.2.28
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 138.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Error condition occurs

Additional Notes

Followed the steps with 6.7 branch

During npm run sync-gutenberg-packages -- --dist-tag=wp-6.8 the error is still there as expected

tools/release/sync-gutenberg-packages.js:110
        const versionConflicts = Object.entries( packageLock.dependencies )


TypeError: Cannot convert undefined or null to object
    at Function.entries (<anonymous>)
    at getMismatchedNonWordPressDependencies (/home/user/src/wordpress/wordpress-develop/tools/release/sync-gutenberg-packages.js:110:34)
    at main (/home/user/src/wordpress/wordpress-develop/tools/release/sync-gutenberg-packages.js:36:28)
    at Object.<anonymous> (/home/user/src/wordpress/wordpress-develop/tools/release/sync-gutenberg-packages.js:225:1)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
    at node:internal/main/run_main_module:28:49

But what has changed is that the execution ended in a warning I can't remember in my previous run:

Running "verify:source-maps" task
Warning: The build/wp-includes/js/dist/data.js file must not contain a sourceMappingURL. Use --force to continue.

Aborted due to warnings

So I run with npm run sync-gutenberg-packages --force -- --dist-tag=wp-6.8 and still fails with the same warning 🤔 (I also removed the build/ dir before doing this to double check if there could be a conflict for some reason)

Anyway I ignored this, and I went for the postsync and finally again the dedupe

I can confirm there are dupes but only 2:

  1. pify
  2. make-dir

In fact, in the first dedupe run, I can't really see any dupes.

Maybe the problem is specifically related to those two packages that are somehow messing around? @desrosj have you found more dupes that these two?

Note: See TracTickets for help on using tickets.