feat: support to run multi script commands specified with regex selector #5871
+334
−39
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@pnpm/plugin-commands-script-runners": minor | ||
| --- | ||
|
|
||
| feat: support wildcard selector to specify multiple scripts to execute" | ||
|
There was a problem hiding this comment. add more details. This text is going to be on the release page. |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,15 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function tryBuildRegExpFromCommand ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| command: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): RegExp | null { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (command.length < 3) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (command[0] !== '/' || command.lastIndexOf('/') < 1) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new RegExp(command.slice(0, command.lastIndexOf('/')).slice(1)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Support
Suggested change
Ref: There was a problem hiding this comment. There are other flac chalactor to present flag https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/flags other than There was a problem hiding this comment. Good catch! I have modified the code: // https://github.com/stdlib-js/regexp-regexp/blob/6428051ac9ef7c9d03468b19bdbb1dc6fc2a5509/lib/regexp.js
const isRegExpStr = /^\/((?:\\\/|[^\/])+)\/([dgimuys]*)$/.test(command)
if (!isRegExpStr) {
return null
}
// https://stackoverflow.com/a/874742/6596777
const match = command.match(new RegExp('^/(.*?)/([dgimuys]*)$'))
if (!match) {
return null
}
try {
return new RegExp(match[1], match[2])
} catch {
return null
}Result: console.log(/build:.*/dgimuys.test("build:web")) // trueExcess flags do not affect the results. There was a problem hiding this comment. In what case would regex flags be useful? A regex is already more powerful than it needs to be for this simple task of selecting scripts. There was a problem hiding this comment. Actually, only the We need to discuss what will happen if the user types
There was a problem hiding this comment.
// https://github.com/stdlib-js/regexp-regexp/blob/6428051ac9ef7c9d03468b19bdbb1dc6fc2a5509/lib/regexp.js
const reRegExp = new RegExp(/^\/((?:\\\/|[^\/])+)\/([dgimuys]*)$/)
const match = command.match(reRegExp)
if (!match) {
return null
}
// if need print warning
if (match[2]) {
logger.warn("not support flags and will ignore")
}
try {
return new RegExp(match[1])
} catch {
return null
}
// https://github.com/stdlib-js/regexp-regexp/blob/6428051ac9ef7c9d03468b19bdbb1dc6fc2a5509/lib/regexp.js
const reRegExp = new RegExp(/^\/((?:\\\/|[^\/])+)\/([dgimuys]*)$/)
const match = command.match(reRegExp)
if (!match) {
return null
}
try {
return new RegExp(match[1], match[2])
} catch {
return null
}There was a problem hiding this comment. I'd vote for either reporting an error or consider it as script name. Although the second one may be confusing There was a problem hiding this comment. I would like to vote for reporting an error or ignoring flag and warning it. There was a problem hiding this comment. I'd vote for ignoring the flags and warning it. There was a problem hiding this comment. I updated the |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BlackHole1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,6 @@ import { RecursiveSummary, throwOnCommandFail } from '@pnpm/cli-utils' | |
| import { Config } from '@pnpm/config' | ||
| import { PnpmError } from '@pnpm/error' | ||
| import { | ||
| makeNodeRequireOption, | ||
| RunLifecycleHookOptions, | ||
| } from '@pnpm/lifecycle' | ||
|
|
@@ -13,6 +12,9 @@ import pLimit from 'p-limit' | |
| import realpathMissing from 'realpath-missing' | ||
| import { existsInDir } from './existsInDir' | ||
| import { getResumedPackageChunks } from './exec' | ||
| import { runScript } from './run' | ||
| import { tryBuildRegExpFromCommand } from './regexpCommand' | ||
| import { PackageScripts } from '@pnpm/types' | ||
|
|
||
| export type RecursiveRunOpts = Pick<Config, | ||
| | 'enablePrePostScripts' | ||
|
|
@@ -73,15 +75,22 @@ export async function runRecursive ( | |
| const missingScriptPackages: string[] = packageChunks | ||
| .flat() | ||
| .map((prefix) => opts.selectedProjectsGraph[prefix]) | ||
| .filter((pkg) => getSpecifiedScripts(pkg.package.manifest.scripts ?? {}, scriptName).length < 1) | ||
| .map((pkg) => pkg.package.manifest.name ?? pkg.package.dir) | ||
| if (missingScriptPackages.length) { | ||
| throw new PnpmError('RECURSIVE_RUN_NO_SCRIPT', `Missing script "${scriptName}" in packages: ${missingScriptPackages.join(', ')}`) | ||
| } | ||
| } | ||
|
|
||
| for (const chunk of packageChunks) { | ||
| const selectedScripts = chunk.map(prefix => { | ||
| const pkg = opts.selectedProjectsGraph[prefix] | ||
| const specifiedScripts = getSpecifiedScripts(pkg.package.manifest.scripts ?? {}, scriptName) | ||
|
|
||
| return specifiedScripts.map(script => ({ prefix, scriptName: script })) | ||
| }).flat() | ||
|
|
||
| await Promise.all(selectedScripts.map(async ({ prefix, scriptName }) => | ||
| limitRun(async () => { | ||
| const pkg = opts.selectedProjectsGraph[prefix] | ||
| if ( | ||
|
|
@@ -113,21 +122,9 @@ export async function runRecursive ( | |
| ...makeNodeRequireOption(pnpPath), | ||
| } | ||
| } | ||
|
|
||
| const _runScript = runScript.bind(null, { manifest: pkg.package.manifest, lifecycleOpts, runScriptOptions: { enablePrePostScripts: opts.enablePrePostScripts ?? false }, passedThruArgs }) | ||
| await _runScript(scriptName) | ||
| result.passes++ | ||
| } catch (err: any) { // eslint-disable-line | ||
| logger.info(err) | ||
|
|
@@ -164,3 +161,18 @@ export async function runRecursive ( | |
|
|
||
| throwOnCommandFail('pnpm recursive run', result) | ||
| } | ||
|
|
||
| function getSpecifiedScripts (scripts: PackageScripts, scriptName: string) { | ||
|
There was a problem hiding this comment. why is this function not the same as in There was a problem hiding this comment. In my understanding, There was a problem hiding this comment. Perhaps we could add a function parameter for controlling this behavior? There was a problem hiding this comment. No parameter is needed. It is better to write another wrapper for There was a problem hiding this comment. I fixed the code to wrap |
||
| const scriptSelector = tryBuildRegExpFromCommand(scriptName) | ||
|
|
||
| if (scriptSelector) { | ||
| const scriptKeys = Object.keys(scripts) | ||
| return scriptKeys.filter(script => script.match(scriptSelector)) | ||
| } | ||
|
|
||
| if (scripts[scriptName]) { | ||
| return [scriptName] | ||
| } | ||
|
|
||
| return [] | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -411,7 +411,7 @@ test('if a script is not found but is present in the root, print an info message | |
| } | ||
|
|
||
| expect(err).toBeTruthy() | ||
| expect(err.hint).toMatch(/But script matched with build is present in the root/) | ||
| }) | ||
|
|
||
| test('scripts work with PnP', async () => { | ||
|
|
@@ -465,3 +465,100 @@ test('pnpm run with custom shell', async () => { | |
|
|
||
| expect((await import(path.resolve('shell-input.json'))).default).toStrictEqual(['-c', 'foo bar']) | ||
| }) | ||
|
|
||
| test('pnpm run with multiple script selector should work', async () => { | ||
| prepare({ | ||
| scripts: { | ||
| 'build:a': 'node -e "require(\'fs\').writeFileSync(\'./output-build-a.txt\', \'a\', \'utf8\')"', | ||
| 'build:b': 'node -e "require(\'fs\').writeFileSync(\'./output-build-b.txt\', \'b\', \'utf8\')"', | ||
| 'build:c': 'node -e "require(\'fs\').writeFileSync(\'./output-build-c.txt\', \'c\', \'utf8\')"', | ||
| build: 'node -e "require(\'fs\').writeFileSync(\'./output-build-a.txt\', \'should not run\', \'utf8\')"', | ||
| 'lint:a': 'node -e "require(\'fs\').writeFileSync(\'./output-lint-a.txt\', \'a\', \'utf8\')"', | ||
| 'lint:b': 'node -e "require(\'fs\').writeFileSync(\'./output-lint-b.txt\', \'b\', \'utf8\')"', | ||
| 'lint:c': 'node -e "require(\'fs\').writeFileSync(\'./output-lint-c.txt\', \'c\', \'utf8\')"', | ||
| lint: 'node -e "require(\'fs\').writeFileSync(\'./output-lint-a.txt\', \'should not run\', \'utf8\')"', | ||
| }, | ||
| }) | ||
|
|
||
| await run.handler({ | ||
| dir: process.cwd(), | ||
| extraBinPaths: [], | ||
| extraEnv: {}, | ||
| rawConfig: {}, | ||
| }, ['/^(lint|build):.*/']) | ||
|
|
||
| expect(await fs.readFile('output-build-a.txt', { encoding: 'utf-8' })).toEqual('a') | ||
| expect(await fs.readFile('output-build-b.txt', { encoding: 'utf-8' })).toEqual('b') | ||
| expect(await fs.readFile('output-build-c.txt', { encoding: 'utf-8' })).toEqual('c') | ||
|
|
||
| expect(await fs.readFile('output-lint-a.txt', { encoding: 'utf-8' })).toEqual('a') | ||
| expect(await fs.readFile('output-lint-b.txt', { encoding: 'utf-8' })).toEqual('b') | ||
| expect(await fs.readFile('output-lint-c.txt', { encoding: 'utf-8' })).toEqual('c') | ||
| }) | ||
|
|
||
| test('pnpm run with multiple script selector should work also for pre/post script', async () => { | ||
| prepare({ | ||
| scripts: { | ||
| 'build:a': 'node -e "require(\'fs\').writeFileSync(\'./output-a.txt\', \'a\', \'utf8\')"', | ||
| 'prebuild:a': 'node -e "require(\'fs\').writeFileSync(\'./output-pre-a.txt\', \'pre-a\', \'utf8\')"', | ||
| }, | ||
| }) | ||
|
|
||
| await run.handler({ | ||
| dir: process.cwd(), | ||
| extraBinPaths: [], | ||
| extraEnv: {}, | ||
| rawConfig: {}, | ||
| enablePrePostScripts: true, | ||
| }, ['/build:.*/']) | ||
|
|
||
| expect(await fs.readFile('output-a.txt', { encoding: 'utf-8' })).toEqual('a') | ||
| expect(await fs.readFile('output-pre-a.txt', { encoding: 'utf-8' })).toEqual('pre-a') | ||
| }) | ||
|
|
||
| test('pnpm run with multiple script selector should work parallel as a default behavior (parallel execution limits number is four)', async () => { | ||
| prepare({ | ||
| scripts: { | ||
| 'build:a': 'node -e "let i = 20;setInterval(() => {if (!--i) process.exit(0); require(\'json-append\').append(Date.now(),\'./output-a.json\');},50)"', | ||
| 'build:b': 'node -e "let i = 40;setInterval(() => {if (!--i) process.exit(0); require(\'json-append\').append(Date.now(),\'./output-b.json\');},25)"', | ||
| }, | ||
| }) | ||
|
|
||
| await execa('pnpm', ['add', 'json-append@1']) | ||
|
|
||
| await run.handler({ | ||
| dir: process.cwd(), | ||
| extraBinPaths: [], | ||
| extraEnv: {}, | ||
| rawConfig: {}, | ||
| }, ['/build:.*/']) | ||
|
|
||
| const { default: outputsA } = await import(path.resolve('output-a.json')) | ||
| const { default: outputsB } = await import(path.resolve('output-b.json')) | ||
|
|
||
| expect(Math.max(outputsA[0], outputsB[0]) < Math.min(outputsA[outputsA.length - 1], outputsB[outputsB.length - 1])).toBeTruthy() | ||
| }) | ||
|
|
||
| test('pnpm run with multiple script selector should work sequentially with --workspace-concurrency=1', async () => { | ||
| prepare({ | ||
| scripts: { | ||
| 'build:a': 'node -e "let i = 2;setInterval(() => {if (!i--) process.exit(0); require(\'json-append\').append(Date.now(),\'./output-a.json\');},16)"', | ||
| 'build:b': 'node -e "let i = 2;setInterval(() => {if (!i--) process.exit(0); require(\'json-append\').append(Date.now(),\'./output-b.json\');},16)"', | ||
| }, | ||
| }) | ||
|
|
||
| await execa('pnpm', ['add', 'json-append@1']) | ||
|
|
||
| await run.handler({ | ||
| dir: process.cwd(), | ||
| extraBinPaths: [], | ||
| extraEnv: {}, | ||
| rawConfig: {}, | ||
| workspaceConcurrency: 1, | ||
| }, ['/build:.*/']) | ||
|
|
||
| const { default: outputsA } = await import(path.resolve('output-a.json')) | ||
| const { default: outputsB } = await import(path.resolve('output-b.json')) | ||
|
|
||
| expect(outputsA[0] < outputsB[0] && outputsA[1] < outputsB[1]).toBeTruthy() | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also add
pnpm: minorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4802e46 👍