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
Conversation
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.
Yes, pnpm run
also needs it.
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)) |
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.
Shouldn't this be an equal check?
const chunkPosition = chunks.findIndex(chunk => chunk.includes(resumeFromPackagePrefix)) | |
const chunkPosition = chunks.findIndex(chunk => chunk === resumeFromPackagePrefix) |
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.
chunks
looks like a two-dimensional array, for example:
[
[
'packages/bar',
'/packages/foo'
],
[
'packages/qar',
'/packages/zoo'
],
]
chunkPosition
n 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.
|
||
if (opts.resumeFrom) { | ||
const resumeFromPackagePrefix = Object.keys(opts.selectedProjectsGraph) | ||
.find((prefix) => opts.selectedProjectsGraph?.[prefix]?.package.manifest.name === opts.resumeFrom) |
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.
if opts.selectedProjectsGraph
is not defined, I think an error should be thrown
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.
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.
but you do check if it is defined by using optional chaining (opts.selectedProjectsGraph?
)
Instead, you can check if it exists before line 132
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.
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
Okay, I will update later |
This test is unstable: |
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.
The new tests should be stable.
I see, it looks like the output order of project-2 and project-3 is not fixed |
close #4690
Implement
--resume-from
forpnpm exec
command.Not sure if
pnpm run
command also need this option, I personally think it is also quite useful, if it is okay, I can also update to support it :)