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

Conversation

await-ovo
Copy link
Member

close #4690

Implement --resume-from for pnpm 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 :)

Copy link
Member

@zkochan zkochan left a 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))
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.


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

@await-ovo
Copy link
Member Author

await-ovo commented Dec 31, 2022

Yes, pnpm run also needs it.

Okay, I will update later

@zkochan
Copy link
Member

zkochan commented Jan 1, 2023

This test is unstable: pnpm -r --resume-from run should executed from given package

Copy link
Member

@zkochan zkochan left a 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.

@await-ovo
Copy link
Member Author

The new tests should be stable.

I see, it looks like the output order of project-2 and project-3 is not fixed

@zkochan zkochan merged commit da15828 into pnpm:main Jan 2, 2023
gluxon pushed a commit to gluxon/pnpm that referenced this pull request Jan 3, 2023
@zkochan zkochan added this to the v7.22 milestone Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement --resume-from [package-name]
2 participants