Skip to content

fix: pnpm recursive update for workspace project#36586

Merged
viceice merged 4 commits intorenovatebot:mainfrom
eboureau:fix/pnpm-workspace-recursive-update
Jun 26, 2025
Merged

fix: pnpm recursive update for workspace project#36586
viceice merged 4 commits intorenovatebot:mainfrom
eboureau:fix/pnpm-workspace-recursive-update

Conversation

@eboureau
Copy link
Copy Markdown
Contributor

@eboureau eboureau commented Jun 18, 2025 •
edited by viceice
Loading

Changes

add --recursive if the project is a pnpm workspace

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@RahulGautamSingh
Copy link
Copy Markdown
Collaborator

RahulGautamSingh commented Jun 18, 2025

Thank you for your PR!

IIRC there's a discussion for this. Could you please add the link for it in the Context section of you PR description?

rarkins
rarkins reviewed Jun 19, 2025
View reviewed changes
Comment thread lib/modules/manager/npm/post-update/pnpm.ts
rarkins
rarkins reviewed Jun 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

This is an analysis from ChatGPT o3:

image

@rarkins
Copy link
Copy Markdown
Contributor

rarkins commented Jun 19, 2025

image

@rarkins
Copy link
Copy Markdown
Contributor

rarkins commented Jun 19, 2025

Per the above ChatGPT analysis:

  • Nothing changed in pnpm v9, so the "if v9 or higher" logic we have in main branch seems wrong if so
  • We shouldn't normally need to add the recursive flag anyway, because recursive is default

@rarkins
Copy link
Copy Markdown
Contributor

rarkins commented Jun 19, 2025

@zkochan could you kindly advise us on if/when Renovate should add the --recursive flag?

It was first added to every pnpm install command about 5 years ago in #6850 which you commented at the time was ok

viceice
viceice requested changes Jun 19, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

see comment

@eboureau
Copy link
Copy Markdown
Contributor Author

eboureau commented Jun 19, 2025 •
edited
Loading

Hi @viceice @rarkins this is true for the install command but not for update (could be a pnpm bug?).
The source code is the source of the truth 😃
https://github.com/pnpm/pnpm/blob/c060387c69555f4d8b704aa187100aec25463417/pnpm/src/main.ts#L178-L190

@viceice
Copy link
Copy Markdown
Member

viceice commented Jun 21, 2025

Hi @viceice @rarkins this is true for the install command but not for update (could be a pnpm bug?).
The source code is the source of the truth 😃
https://github.com/pnpm/pnpm/blob/c060387c69555f4d8b704aa187100aec25463417/pnpm/src/main.ts#L178-L190

so it's a pnpm bug after all which doesn't automatically add the recursive flag in our update case. 😥

@eboureau
Copy link
Copy Markdown
Contributor Author

eboureau commented Jun 22, 2025

Hi @viceice @rarkins this is true for the install command but not for update (could be a pnpm bug?).
The source code is the source of the truth 😃
https://github.com/pnpm/pnpm/blob/c060387c69555f4d8b704aa187100aec25463417/pnpm/src/main.ts#L178-L190

so it's a pnpm bug after all which doesn't automatically add the recursive flag in our update case. 😥

I don't know if it's a bug but at least it does not follow the same rule as the install command which I agree is quite confusing. The update doc is mentioning the --recursive option.

@eboureau
Copy link
Copy Markdown
Contributor Author

eboureau commented Jun 26, 2025 •
edited
Loading

Hi @viceice @rarkins,
Are you happy with my latest changes?
Any other comments or can we get it merged?

@viceice viceice added the auto:no-done-comments Don't say "Done" or "Please review" - request a review instead label Jun 26, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 26, 2025

Hi there,

You are using done comments which cause a lot of noise/notifications. Instead, please use GitHub's web interface to request another review. Please read our contributing guidelines to reduce noise.

Good luck,

The Renovate team

@viceice viceice added this pull request to the merge queue Jun 26, 2025
Merged via the queue into renovatebot:main with commit 1bef8cc Jun 26, 2025
41 checks passed
@renovate-release
Copy link
Copy Markdown

renovate-release commented Jun 26, 2025

🎉 This PR is included in version 41.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@viceice viceice viceice approved these changes

+1 more reviewer

@rarkins rarkins rarkins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

auto:no-done-comments Don't say "Done" or "Please review" - request a review instead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@eboureau @RahulGautamSingh @rarkins @viceice @renovate-release