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

Remove most usages of npx #64148

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Remove most usages of npx #64148

wants to merge 1 commit into from

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented May 30, 2022

Inspired by some changes in #62003 that @noahtallen did, this PR removes most usages of npx from Calypso scripts, and replaces them with some form of yarn run or yarn run -T.

The rules I'm following are:

  • if the package that provides the binary is a direct dependency of a package, the binary can be invoked with yarn run, no matter in which node_modules/.bin directory it actually lives after hoisting. Yarn will find it.
  • if the package that provides the binary is a dependency of the root package.json, then we can invoke it with yarn run -T.

@jsnajdr jsnajdr requested a review from noahtallen as a code owner May 30, 2022 12:45
@jsnajdr jsnajdr self-assigned this May 30, 2022
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 30, 2022
@github-actions
Copy link

github-actions bot commented May 30, 2022

@jsnajdr jsnajdr force-pushed the remove/npx-usages branch from 8f19b88 to f071ef2 Compare May 30, 2022 12:48
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@jsnajdr
Copy link
Member Author

jsnajdr commented May 30, 2022

The apps/happychat and apps/inline-help builds are failing with:

Usage Error: Couldn't find a script name "rimraf" in the top-level (used by @automattic/inline-help@workspace:apps/inline-help).

which is strange because rimraf is declared and installed in the top-level package.json.

@noahtallen
Copy link
Contributor

Oh that's interesting. I think that could be because the TeamCity build for these apps uses yarn workspace focus to avoid having to yarn install the entire repo. It seems very plausible that focusing on a single workspace won't link any dependencies (and therefore bin scripts) from the top-level node_modules.

I'm guessing we can fix it by adding rimraf as a dev dependency locally. IIRC this is what scinos had been doing in a lot of cases: just explicitly adding every single dependency in every package which needed it after running yarn workspace focus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants