WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 9 days 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 3 weeks ago.

Download all attachments as: .zip

Change History (20)

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


5 weeks ago

  • Keywords has-patch has-unit-tests added

#2 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Awaiting Review to 5.8

#3 @desrosj
5 weeks 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
5 weeks 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
5 weeks 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
5 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#7 @desrosj
5 weeks 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
5 weeks 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
5 weeks 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
4 weeks 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
4 weeks 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
3 weeks 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 3 weeks ago by SergeyBiryukov (previous) (diff)

#13 @SergeyBiryukov
3 weeks 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
3 weeks 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
3 weeks ago

#15 @desrosj
3 weeks 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
3 weeks 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
3 weeks ago

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

#18 @desrosj
3 weeks 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
9 days 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.

Note: See TracTickets for help on using tickets.