WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 6 months ago

#52843 closed defect (bug) (fixed)

Remove explicit puppeteer dependency from package.json

Reported by: isabel_brison Owned by: desrosj
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The explicit dependency was added here: https://github.com/WordPress/wordpress-develop/pull/347/files because puppeteer wasn't being properly installed as a dependency of the @wordpress/scripts package.

In the meantime, the version in package.json has fallen out of sync with @wordpress/scripts, so I tried removing it. After deleting node_modules and the lock file, and reinstalling everything, the tests are running without errors.

It's easier for maintainability if we don't have to update versions in two different places, so I propose removing puppeteer from package.json altogether.

Attachments (1)

52843.diff (4.0 KB) - added by desrosj 8 months ago.

Download all attachments as: .zip

Change History (21)

This ticket was mentioned in PR #1102 on WordPress/wordpress-develop by tellthemachines.


8 months ago

  • Keywords has-patch has-unit-tests added

#2 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 5.8

#3 @desrosj
8 months ago

Thanks @isabel_brison! This makes sense to me!

Do you know when the dependency issue was fixed? This may be worth backporting to simplify the dependency list, but older branches don't all run the latest version of @wordpress/wp-scripts.

#4 @desrosj
8 months ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 50540:

Build/Test Tools: Remove explicit puppeteer dependency.

This was added in [48177] to fix an issue where Puppeteer was not being installed correctly as a dependency of @wordpress/wp-scripts. This has been fixed, so this explicit dependency can be removed.

Props isabel_brison.
Fixes #52843.

#5 @prbot
8 months ago

desrosj commented on PR #1102:

Merged into Core in https://core.trac.wordpress.org/changeset/50540. Just to note, there are a bunch of unrelated nested dependency updates here that I did not include.

#6 @desrosj
8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Looks like [50540] broke the E2E workflow. Investigating.

#7 @desrosj
8 months ago

I'm going to revert [50540] for the time being. I'm not sure why, but applying the diff from the PR verbatim locally is producing a Failed to load resource: the server responded with a status of 500 (Internal Server Error) error as well.

#8 @desrosj
8 months ago

In 50542:

Build/Test Tools: Revert [50540].

The E2E workflow is failing after this change. Reverting to to investigate further.

Unprops desrosj.
See #52843.

#9 @isabel_brison
8 months ago

@desroj, did you regenerate the the package-lock before committing? I notice that your commit just removes the root puppeteer dependency on line 18646, but the package-lock diff in my PR shows it instead to be replaced by the updated version: https://github.com/WordPress/wordpress-develop/pull/1102/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519L18646, and the nested dependency of wp-scripts being removed: https://github.com/WordPress/wordpress-develop/pull/1102/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519L3915

I only got it to work locally after deleting my node_modules and re-installing, so you may have to do that too. In any case, to avoid errors it's better to commit the whole package-lock changeset.

#10 @desrosj
8 months ago

@isabel_brison I tried deleting node_modules, and even erasing my package-lock.json file and regenerating on npm install, but I'm still seeing the test fail for me locally. Not sure what I'm missing.

#11 @isabel_brison
8 months ago

Hmmm, that's weird. Which version of node/npm are you on? Mine is node: 14.15.4 npm: 6.14.10. I've also tested on a Windows 10 PC with node: 12.16.1 and npm: 6.14.5 and it worked well after deleting node_modules :/

If you look into your node_modules do you see the puppeteer folder in the root directory?

#12 @SergeyBiryukov
8 months ago

Just noting that with node 12.17.0 and npm 6.14.5 I initially got the same package-lock.json file as in [50540] when following the steps from comment:10.

After running npm cache clean -f and updating to node 14.16.0 and npm 6.14.11, I got a slightly different package-lock.json file, and with that one the E2E workflow seems to pass.

Let's try this again and see if it works :)

Last edited 8 months ago by SergeyBiryukov (previous) (diff)

#13 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50612:

Build/Test Tools: Remove explicit puppeteer dependency.

This was added in [48177] to fix an issue where Puppeteer was not being installed correctly as a dependency of @wordpress/wp-scripts. This has been fixed, so this explicit dependency can be removed.

Props isabel_brison, desrosj, SergeyBiryukov.
Fixes #52843.

#14 @desrosj
8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm still seeing some errors locally when running npm run test e2e:

> node ./tests/e2e/run-tests.js

Chromium is already in /Users/username/Sites/wordpress-develop/node_modules/@wordpress/scripts/node_modules/puppeteer/.local-chromium/mac-818858; skipping download.
Error: Cannot find module 'puppeteer'
Require stack:
- /Users/username/Sites/wordpress-develop/node_modules/jest-environment-puppeteer/lib/readConfig.js
- /Users/username/Sites/wordpress-develop/node_modules/jest-environment-puppeteer/lib/global.js
- /Users/username/Sites/wordpress-develop/node_modules/jest-environment-puppeteer/setup.js
- /Users/username/Sites/wordpress-develop/node_modules/@jest/transform/build/ScriptTransformer.js
- /Users/username/Sites/wordpress-develop/node_modules/@jest/transform/build/index.js
- /Users/username/Sites/wordpress-develop/node_modules/jest-runtime/build/index.js
- /Users/username/Sites/wordpress-develop/node_modules/@jest/core/build/cli/index.js
- /Users/username/Sites/wordpress-develop/node_modules/@jest/core/build/jest.js
- /Users/username/Sites/wordpress-develop/node_modules/jest/node_modules/jest-cli/build/cli/index.js
- /Users/username/Sites/wordpress-develop/node_modules/jest/node_modules/jest-cli/build/index.js
- /Users/username/Sites/wordpress-develop/node_modules/jest/build/jest.js
- /Users/username/Sites/wordpress-develop/node_modules/@wordpress/scripts/scripts/test-e2e.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:880:15)
    at Function.Module._load (internal/modules/cjs/loader.js:725:27)
    at Module.require (internal/modules/cjs/loader.js:952:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at getPuppeteer (/Users/username/Sites/wordpress-develop/node_modules/jest-environment-puppeteer/lib/readConfig.js:58:14)
    at setup (/Users/username/Sites/wordpress-develop/node_modules/jest-environment-puppeteer/lib/global.js:21:50)
    at async /Users/username/Sites/wordpress-develop/node_modules/@jest/core/build/runGlobalHook.js:82:11
    at async waitForPromiseWithCleanup (/Users/username/Sites/wordpress-develop/node_modules/@jest/transform/build/ScriptTransformer.js:207:5)
    at async /Users/username/Sites/wordpress-develop/node_modules/@jest/core/build/runGlobalHook.js:72:9
    at async pEachSeries (/Users/username/Sites/wordpress-develop/node_modules/p-each-series/index.js:8:23)
child_process.js:655
    throw err;
    ^

Error: Command failed: wp-scripts test-e2e --config tests/e2e/jest.config.js
    at checkExecSyncError (child_process.js:616:11)
    at execSync (child_process.js:652:15)
    at Object.<anonymous> (/Users/username/Sites/wordpress-develop/tests/e2e/run-tests.js:9:1)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  status: 1,
  signal: null,
  output: [ null, null, null ],
  pid: 77038,
  stdout: null,
  stderr: null
}

When I run npm list puppeteer, I get the following output:

WordPress@5.8.0 /Users/username/Sites/wordpress-develop
├─┬ @wordpress/scripts@13.0.3
│ └── UNMET PEER DEPENDENCY puppeteer@npm:puppeteer-core@5.5.0
└─┬ grunt-contrib-qunit@4.0.0
  └── puppeteer@4.0.1

npm ERR! peer dep missing: puppeteer@>= 1.5.0 < 3, required by jest-puppeteer@4.4.0

I am on MacOS running Node 14.16.0/NPM 6.14.11. I removed my node_modules folder and ran npm install fresh and still get that issue.

@desrosj
8 months ago

#15 @desrosj
8 months ago

Doing some more debugging, it seems that my lock file is being updated to remove the dependency when running npm install. 52843.diff has the changes I'm seeing.

#16 @isabel_brison
8 months ago

@desrosj did you clear your npm cache before reinstalling?

So far I've only been able to reproduce your issue after symlinking my local @wordpress/scripts package to wordpress-develop. After doing that, deleting node_modules and reinstalling produced exactly the same diff as yours, but repeating the process installed correctly.

#17 @isabel_brison
8 months ago

@desrosj I'm wondering if you could have something in your user .npmrc that's stopping npm from flattening dependencies?

#18 @desrosj
8 months ago

More testing results:

  • Ran npm cache clean --force.
  • rm -rf node_modules
  • npm install

Still see the issue and running npm list puppeteer still shows the unmet peer dependency.

I was playing around with v2 format of the lock file today for #52951, and oddly enough when the lock file is converted to the new v2 format, I don't have this problem. Maybe updating the file format is enough to resolve this?

#19 @desrosj
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

I'm not sure why, but I'm no longer seeing the problems above locally. Going to close this out.

#20 @prbot
6 months ago

tellthemachines commented on PR #1102:

Closing as this has been committed.

Note: See TracTickets for help on using tickets.