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(plugin-commands-script-runners): support --resume-from for pnpm exec command #5856

Merged
merged 5 commits into from
Jan 2, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/silly-ties-arrive.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@pnpm/plugin-commands-script-runners": minor
"pnpm": minor
---

the `pnpm exec` command support `--resume-from` option. When used, the command will executed from given package [#4690](https://github.com/pnpm/pnpm/issues/4690).
20 changes: 20 additions & 0 deletions exec/plugin-commands-script-runners/src/exec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
PARALLEL_OPTION_HELP,
shorthands as runShorthands,
} from './run'
import { PnpmError } from '@pnpm/error'

export const shorthands = {
parallel: runShorthands.parallel,
Expand All @@ -34,6 +35,7 @@ export function rcOptionsTypes () {
'workspace-concurrency',
], types),
'shell-mode': Boolean,
'resume-from': String,
}
}

Expand Down Expand Up @@ -66,6 +68,10 @@ The shell should understand the -c switch on UNIX or /d /s /c on Windows.',
name: '--shell-mode',
shortAlias: '-c',
},
{
description: 'command executed from given package',
name: '--resume-from',
},
],
},
],
Expand All @@ -83,6 +89,7 @@ export async function handler (
sort?: boolean
workspaceConcurrency?: number
shellMode?: boolean
resumeFrom?: string
} & Pick<Config, 'extraBinPaths' | 'extraEnv' | 'lockfileDir' | 'dir' | 'userAgent' | 'recursive' | 'workspaceDir'>,
params: string[]
) {
Expand Down Expand Up @@ -120,6 +127,19 @@ export async function handler (
}
}
}

if (opts.resumeFrom) {
const resumeFromPackagePrefix = Object.keys(opts.selectedProjectsGraph)
.find((prefix) => opts.selectedProjectsGraph?.[prefix]?.package.manifest.name === opts.resumeFrom)
Copy link
Member

Choose a reason for hiding this comment

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

if opts.selectedProjectsGraph is not defined, I think an error should be thrown

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not quite sure about this situation, can you help explain why it is not defined? thanks ~

I looked at the type definition and it doesn't seem to be undefined?
image

Copy link
Member

Choose a reason for hiding this comment

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

but you do check if it is defined by using optional chaining (opts.selectedProjectsGraph?)

Instead, you can check if it exists before line 132

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, I'm using the optional chain based on the usage in line 159.

I wonder if we can remove the optional chaining if it really won't be undefined, I don't see other examples of checking opts.selectedProjectsGraph elsewhere, like in run or runRecursive, Please let me know if there is something missing. Thanks a lot


if (!resumeFromPackagePrefix) {
throw new PnpmError('RECURSIVE_EXEC_FAIL', `Cannot find package ${opts.resumeFrom}. Could not determine where to resume from.`)
}

const chunkPosition = chunks.findIndex(chunk => chunk.includes(resumeFromPackagePrefix))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be an equal check?

Suggested change
const chunkPosition = chunks.findIndex(chunk => chunk.includes(resumeFromPackagePrefix))
const chunkPosition = chunks.findIndex(chunk => chunk === resumeFromPackagePrefix)

Copy link
Member Author

@await-ovo await-ovo Dec 31, 2022

Choose a reason for hiding this comment

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

chunks looks like a two-dimensional array, for example:

[
  [
    'packages/bar',
    '/packages/foo'
  ],
  [
    'packages/qar',
    '/packages/zoo'
  ],
]

chunkPositionn I use to determine the index of this package in the chunks array.
Since the packages inside the chunk will be executed in parallel, it is not easy to determine the order, so i think it seems to be possible to re-execute directly from the chunkPosition.

chunks = chunks.slice(chunkPosition)
}

const existsPnp = existsInDir.bind(null, '.pnp.cjs')
const workspacePnpPath = opts.workspaceDir && await existsPnp(opts.workspaceDir)

Expand Down
109 changes: 109 additions & 0 deletions exec/plugin-commands-script-runners/test/exec.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -552,3 +552,112 @@ testOnPosixOnly('pnpm recursive exec works with PnP', async () => {
expect(outputs1).toStrictEqual(['project-1', 'project-2-prebuild', 'project-2', 'project-2-postbuild'])
expect(outputs2).toStrictEqual(['project-1', 'project-3'])
})

test('pnpm recursive exec --resume-from should work', async () => {
preparePackages([
{
name: 'project-1',
version: '1.0.0',
dependencies: {
'json-append': '1',
},
scripts: {
build: 'node -e "process.stdout.write(\'project-1\')" | json-append ../output1.json',
},
},
{
name: 'project-2',
version: '1.0.0',
dependencies: {
'json-append': '1',
'project-1': '1',
},
scripts: {
build: 'node -e "process.stdout.write(\'project-2\')" | json-append ../output1.json',
},
},
{
name: 'project-3',
version: '1.0.0',
dependencies: {
'json-append': '1',
'project-1': '1',
},
scripts: {
build: 'node -e "process.stdout.write(\'project-3\')" | json-append ../output1.json',
},
},
{
name: 'project-4',
version: '1.0.0',
dependencies: {
'json-append': '1',
},
scripts: {
build: 'node -e "process.stdout.write(\'project-4\')" | json-append ../output1.json',
},
},
])

const { selectedProjectsGraph } = await readProjects(process.cwd(), [])
await execa(pnpmBin, [
'install',
'-r',
'--registry',
REGISTRY_URL,
'--store-dir',
path.resolve(DEFAULT_OPTS.storeDir),
])
await exec.handler({
...DEFAULT_OPTS,
dir: process.cwd(),
selectedProjectsGraph,
recursive: true,
sort: true,
resumeFrom: 'project-3',
}, ['npm', 'run', 'build'])

const { default: outputs1 } = await import(path.resolve('output1.json'))
expect(outputs1).not.toContain('project-1')
expect(outputs1).not.toContain('project-4')
expect(outputs1).toContain('project-2')
expect(outputs1).toContain('project-3')
})

test('should throw error when the package specified by resume-from does not exist', async () => {
preparePackages([
{
name: 'foo',
version: '1.0.0',
dependencies: {
'json-append': '1',
},
scripts: {
build: 'echo foo',
},
},
])

const { selectedProjectsGraph } = await readProjects(process.cwd(), [])
await execa(pnpmBin, [
'install',
'-r',
'--registry',
REGISTRY_URL,
'--store-dir',
path.resolve(DEFAULT_OPTS.storeDir),
])

try {
await exec.handler({
...DEFAULT_OPTS,
dir: process.cwd(),
selectedProjectsGraph,
recursive: true,
sort: true,
resumeFrom: 'project-2',
}, ['npm', 'run', 'build'])
} catch (err: any) { // eslint-disable-line
expect(err.code).toBe('ERR_PNPM_RECURSIVE_EXEC_FAIL')
}
})