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

feat: fall back to global npm and yarn if not found #871

Merged
merged 8 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/workflows/onRelease.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,22 @@ jobs:
githubTag: ${{ github.event.release.tag_name || inputs.tag }}
secrets:
NPM_TOKEN: ${{ secrets.NPM_TOKEN }}

build-lite-version:
runs-on: ubuntu-latest
steps:
- name: Build lite version
run: |
# update package.json name to @oclif/plugin-plugins-lite
jq '.name = "@oclif/plugin-plugins-lite"' package.json > temp.json && mv temp.json package.json
yarn remove yarn npm
yarn install

publish-lite-version:
uses: salesforcecli/github-workflows/.github/workflows/npmPublish.yml@main
needs: [getDistTag, npm, build-lite-version]
with:
tag: ${{ needs.getDistTag.outputs.tag || 'latest' }}
githubTag: ${{ github.event.release.tag_name || inputs.tag }}
secrets:
NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
7 changes: 7 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,12 @@ jobs:
matrix:
os: [ubuntu-latest, windows-latest]
test: ['test:integration:install', 'test:integration:link']
lite: [true, false]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's clever

exclude:
- os: windows-latest
test: test:integration:link
- lite: true
test: test:integration:link
runs-on: ${{matrix.os}}
steps:
- uses: actions/checkout@v4
Expand All @@ -53,5 +56,9 @@ jobs:
node-version: latest
- uses: salesforcecli/github-workflows/.github/actions/yarnInstallWithRetries@main
- run: yarn build
- name: Remove package managers
if: ${{matrix.lite}}
run: |
yarn remove yarn npm
- name: Run tests
run: yarn ${{matrix.test}}
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"object-treeify": "^4.0.1",
"semver": "^7.6.2",
"validate-npm-package-name": "^5.0.0",
"which": "^4.0.0",
"yarn": "^1.22.22"
},
"devDependencies": {
Expand All @@ -28,6 +29,7 @@
"@types/semver": "^7.5.8",
"@types/sinon": "^17",
"@types/validate-npm-package-name": "^4.0.2",
"@types/which": "^3.0.3",
"chai": "^4.4.1",
"commitlint": "^18",
"eslint": "^8.56.0",
Expand Down
27 changes: 19 additions & 8 deletions src/npm.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import {Interfaces, ux} from '@oclif/core'
import {Errors, Interfaces, ux} from '@oclif/core'
import makeDebug from 'debug'
import {readFile} from 'node:fs/promises'
import {createRequire} from 'node:module'
import {join, sep} from 'node:path'

import {ExecOptions, Output, fork} from './fork.js'
import {LogLevel} from './log-level.js'
import {ExecOptions, Output, spawn} from './spawn.js'

const debug = makeDebug('@oclif/plugin-plugins:npm')

Expand Down Expand Up @@ -36,7 +36,7 @@ export class NPM {

debug(`${options.cwd}: ${bin} ${args.join(' ')}`)
try {
const output = await fork(bin, args, options)
const output = await spawn(bin, args, options)
debug('npm done')
return output
} catch (error: unknown) {
Expand Down Expand Up @@ -64,17 +64,28 @@ export class NPM {

/**
* Get the path to the npm CLI file.
* This will always resolve npm to the pinned version in `@oclif/plugin-plugins/package.json`.
* This will resolve npm to the pinned version in `@oclif/plugin-plugins/package.json` if it exists.
* Otherwise, it will use the globally installed npm.
*
* @returns The path to the `npm/bin/npm-cli.js` file.
*/
private async findNpm(): Promise<string> {
if (this.bin) return this.bin

const npmPjsonPath = createRequire(import.meta.url).resolve('npm/package.json')
const npmPjson = JSON.parse(await readFile(npmPjsonPath, {encoding: 'utf8'}))
const npmPath = npmPjsonPath.slice(0, Math.max(0, npmPjsonPath.lastIndexOf(sep)))
this.bin = join(npmPath, npmPjson.bin.npm)
try {
const npmPjsonPath = createRequire(import.meta.url).resolve('npm/package.json')
const npmPjson = JSON.parse(await readFile(npmPjsonPath, {encoding: 'utf8'}))
const npmPath = npmPjsonPath.slice(0, Math.max(0, npmPjsonPath.lastIndexOf(sep)))
this.bin = join(npmPath, npmPjson.bin.npm)
} catch {
const {default: which} = await import('which')
this.bin = await which('npm')
}

if (!this.bin) {
throw new Errors.CLIError('npm not found')
}

return this.bin
}
}
3 changes: 1 addition & 2 deletions src/plugins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import {basename, dirname, join, resolve} from 'node:path'
import {fileURLToPath} from 'node:url'
import {gt, valid, validRange} from 'semver'

import {Output} from './fork.js'
import {LogLevel} from './log-level.js'
import {NPM} from './npm.js'
import {Output} from './spawn.js'
import {uniqWith} from './util.js'
import {Yarn} from './yarn.js'

Expand Down Expand Up @@ -330,7 +330,6 @@ export default class Plugins {
}

public async update(): Promise<void> {
// eslint-disable-next-line unicorn/no-await-expression-member
let plugins = (await this.list()).filter((p): p is Interfaces.PJSON.PluginTypes.User => p.type === 'user')
if (plugins.length === 0) return

Expand Down
40 changes: 21 additions & 19 deletions src/fork.ts → src/spawn.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {Errors, ux} from '@oclif/core'
import makeDebug from 'debug'
import {fork as cpFork} from 'node:child_process'
import {spawn as cpSpawn} from 'node:child_process'
import {npmRunPathEnv} from 'npm-run-path'

import {LogLevel} from './log-level.js'
Expand All @@ -15,27 +15,29 @@ export type Output = {
stdout: string[]
}

const debug = makeDebug('@oclif/plugin-plugins:fork')
const debug = makeDebug('@oclif/plugin-plugins:spawn')

export async function fork(modulePath: string, args: string[] = [], {cwd, logLevel}: ExecOptions): Promise<Output> {
export async function spawn(modulePath: string, args: string[] = [], {cwd, logLevel}: ExecOptions): Promise<Output> {
return new Promise((resolve, reject) => {
const forked = cpFork(modulePath, args, {
// On windows, the global path to npm could be .cmd, .exe, or .js. If it's a .js file, we need to run it with node.
if (process.platform === 'win32' && modulePath.endsWith('.js')) {
args.unshift(`"${modulePath}"`)
modulePath = 'node'
}

debug('modulePath', modulePath)
debug('args', args)
const spawned = cpSpawn(modulePath, args, {
cwd,
env: {
...npmRunPathEnv(),
// Disable husky hooks because a plugin might be trying to install them, which will
// break the install since the install location isn't a .git directory.
HUSKY: '0',
},
execArgv: process.execArgv
.join(' ')
// Remove --loader ts-node/esm from execArgv so that the subprocess doesn't fail if it can't find ts-node.
// The ts-node/esm loader isn't need to execute npm or yarn commands anyways.
.replace('--loader ts-node/esm', '')
.replace('--loader=ts-node/esm', '')
.split(' ')
.filter(Boolean),
stdio: [0, null, null, 'ipc'],
stdio: 'pipe',
windowsVerbatimArguments: true,
...(process.platform === 'win32' && modulePath.toLowerCase().endsWith('.cmd') && {shell: true}),
})

const possibleLastLinesOfNpmInstall = ['up to date', 'added']
Expand All @@ -56,8 +58,8 @@ export async function fork(modulePath: string, args: string[] = [], {cwd, logLev
return logLevel !== 'silent'
}

forked.stderr?.setEncoding('utf8')
forked.stderr?.on('data', (d: Buffer) => {
spawned.stderr?.setEncoding('utf8')
spawned.stderr?.on('data', (d: Buffer) => {
const output = d.toString().trim()
stderr.push(output)
if (shouldPrint(output)) {
Expand All @@ -66,8 +68,8 @@ export async function fork(modulePath: string, args: string[] = [], {cwd, logLev
} else debug(output)
})

forked.stdout?.setEncoding('utf8')
forked.stdout?.on('data', (d: Buffer) => {
spawned.stdout?.setEncoding('utf8')
spawned.stdout?.on('data', (d: Buffer) => {
const output = d.toString().trim()
stdout.push(output)
if (shouldPrint(output)) {
Expand All @@ -76,8 +78,8 @@ export async function fork(modulePath: string, args: string[] = [], {cwd, logLev
} else debug(output)
})

forked.on('error', reject)
forked.on('exit', (code: number) => {
spawned.on('error', reject)
spawned.on('exit', (code: number) => {
if (code === 0) {
resolve({stderr, stdout})
} else {
Expand Down
21 changes: 17 additions & 4 deletions src/yarn.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import {Interfaces, ux} from '@oclif/core'
import {Errors, Interfaces, ux} from '@oclif/core'
import makeDebug from 'debug'
import {createRequire} from 'node:module'
import {fileURLToPath} from 'node:url'

import {ExecOptions, Output, fork} from './fork.js'
import {LogLevel} from './log-level.js'
import {ExecOptions, Output, spawn} from './spawn.js'

const require = createRequire(import.meta.url)
const debug = makeDebug('@oclif/plugin-plugins:yarn')

export class Yarn {
private bin: string | undefined
private config: Interfaces.Config
private logLevel: LogLevel

Expand All @@ -31,7 +32,7 @@ export class Yarn {

debug(`${options.cwd}: ${bin} ${args.join(' ')}`)
try {
const output = await fork(bin, args, options)
const output = await spawn(bin, args, options)
debug('yarn done')
return output
} catch (error: unknown) {
Expand All @@ -45,6 +46,18 @@ export class Yarn {
}

private async findYarn(): Promise<string> {
return require.resolve('yarn/bin/yarn.js', {paths: [this.config.root, fileURLToPath(import.meta.url)]})
if (this.bin) return this.bin
try {
this.bin = require.resolve('yarn/bin/yarn.js', {paths: [this.config.root, fileURLToPath(import.meta.url)]})
} catch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the different resolve logic for yarn? When I remove yarn from my linked plugin-plugins it resolves the to yarn that is included with the sf cli rather than falling back to my locally installed version
@oclif/plugin-plugins:yarn yarn binary path /Users/ewillhoit/.nvm/versions/node/v20.12.2/lib/node_modules/@salesforce/cli/node_modules/yarn/bin/yarn.j

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pulled the logic in exactly as it was in the last major: https://github.com/oclif/plugin-plugins/blob/4.3.10/src/yarn.ts#L30

I can replicate the same logic that we use for npm... but I'm wondering if it's worth it if we one day hope to remove the "install dependencies after linking" logic altogether. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine since we'd like to get rid of this. I don't really see an issue with this, I was just curious if it was deliberate.

const {default: which} = await import('which')
this.bin = await which('yarn')
}

if (!this.bin) {
throw new Errors.CLIError('yarn not found')
}

return this.bin
}
}
38 changes: 9 additions & 29 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1994,6 +1994,11 @@
resolved "https://registry.npmjs.org/@types/validate-npm-package-name/-/validate-npm-package-name-4.0.2.tgz"
integrity sha512-lrpDziQipxCEeK5kWxvljWYhUvOiB2A9izZd9B2AFarYAkqZshb4lPbRs7zKEic6eGtH8V/2qJW+dPp9OtF6bw==

"@types/which@^3.0.3":
version "3.0.3"
resolved "https://registry.yarnpkg.com/@types/which/-/which-3.0.3.tgz#41142ed5a4743128f1bc0b69c46890f0453ddb89"
integrity sha512-2C1+XoY0huExTbs8MQv1DuS5FS86+SEjdM9F/+GS61gg5Hqbtj8ZiDSx8MfWcyei907fIPbfPGCOrNUTnVHY1g==

"@types/wrap-ansi@^3.0.0":
version "3.0.0"
resolved "https://registry.yarnpkg.com/@types/wrap-ansi/-/wrap-ansi-3.0.0.tgz#18b97a972f94f60a679fd5c796d96421b9abb9fd"
Expand Down Expand Up @@ -6417,16 +6422,7 @@ [email protected]:
resolved "https://registry.npmjs.org/string-argv/-/string-argv-0.3.2.tgz"
integrity sha512-aqD2Q0144Z+/RqG52NeHEkZauTAUWJO8c6yTftGJKO3Tja5tUgIfmIl6kExvhtxSDP7fXB6DvzkfMpCd/F3G+Q==

"string-width-cjs@npm:string-width@^4.2.0":
version "4.2.3"
resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz"
integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
dependencies:
emoji-regex "^8.0.0"
is-fullwidth-code-point "^3.0.0"
strip-ansi "^6.0.1"

"string-width@^1.0.2 || 2 || 3 || 4", string-width@^4.0.0, string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
"string-width-cjs@npm:string-width@^4.2.0", "string-width@^1.0.2 || 2 || 3 || 4", string-width@^4.0.0, string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
version "4.2.3"
resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz"
integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
Expand Down Expand Up @@ -6487,14 +6483,7 @@ string_decoder@^1.1.1:
dependencies:
safe-buffer "~5.2.0"

"strip-ansi-cjs@npm:strip-ansi@^6.0.1":
version "6.0.1"
resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz"
integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
dependencies:
ansi-regex "^5.0.1"

strip-ansi@^6.0.0, strip-ansi@^6.0.1:
"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1:
version "6.0.1"
resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz"
integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
Expand Down Expand Up @@ -6917,7 +6906,7 @@ which@^2.0.1:

which@^4.0.0:
version "4.0.0"
resolved "https://registry.npmjs.org/which/-/which-4.0.0.tgz"
resolved "https://registry.yarnpkg.com/which/-/which-4.0.0.tgz#cd60b5e74503a3fbcfbf6cd6b4138a8bae644c1a"
integrity sha512-GlaYyEb07DPxYCKhKzplCWBJtvxZcZMrL+4UkrTSJHHPyZU4mYYTv3qaOe77H7EODLSSopAUFAc6W8U4yqvscg==
dependencies:
isexe "^3.1.1"
Expand Down Expand Up @@ -6946,7 +6935,7 @@ [email protected]:
resolved "https://registry.npmjs.org/workerpool/-/workerpool-6.2.1.tgz"
integrity sha512-ILEIE97kDZvF9Wb9f6h5aXK4swSlKGUcOEGiIYb2OOu/IrDU9iwj0fD//SsA6E5ibwJxpEvhullJY4Sl4GcpAw==

"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0":
"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0", wrap-ansi@^7.0.0:
version "7.0.0"
resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz"
integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
Expand All @@ -6964,15 +6953,6 @@ wrap-ansi@^6.2.0:
string-width "^4.1.0"
strip-ansi "^6.0.0"

wrap-ansi@^7.0.0:
version "7.0.0"
resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz"
integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
dependencies:
ansi-styles "^4.0.0"
string-width "^4.1.0"
strip-ansi "^6.0.0"

wrap-ansi@^8.1.0:
version "8.1.0"
resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-8.1.0.tgz"
Expand Down
Loading