Skip to content

Fix workspace member node_modules detection#23264

Closed
jackjennings wants to merge 2 commits intopantsbuild:mainfrom
jackjennings:jack.jennings/node-modules-walker
Closed

Fix workspace member node_modules detection#23264
jackjennings wants to merge 2 commits intopantsbuild:mainfrom
jackjennings:jack.jennings/node-modules-walker

Conversation

@jackjennings
Copy link
Copy Markdown
Contributor

@jackjennings jackjennings commented Apr 15, 2026

I've run into an issue with the JavaScript backend: when a workspace member has its own node_modules (e.g. in the case that there is a version conflict between the member's package version and the root packages) the member's node_modules are present in the Pants sandbox.

The node_modules_directories method in nodejs_project_environment.py was designed to yield workspace member node_modules paths, but the condition if self.package was always the resolve root (because _run_tool_with_resolve always calls install_node_packages_for_address with the root package's address).

Code written with Claude using TDD methodology.

@jackjennings jackjennings changed the title Fix workspace member node_modules detection Fix workspace member node_modules detection Apr 15, 2026
@jackjennings jackjennings force-pushed the jack.jennings/node-modules-walker branch from 9c4e47a to ec80f05 Compare April 15, 2026 23:56
@jackjennings jackjennings marked this pull request as ready for review April 15, 2026 23:57
@tobni tobni self-requested a review April 17, 2026 11:16
Copy link
Copy Markdown
Contributor

@tobni tobni left a comment

Choose a reason for hiding this comment

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

Tricky!

But LGTM. When CI is happy I will approve.

@tobni tobni self-assigned this Apr 17, 2026
@jackjennings
Copy link
Copy Markdown
Contributor Author

Hey @tobni — I'm running into some issues with running pants locally that I haven't been able to resolve (posted about them in the #development slack channel). If this seems valuable to land, feel free to take over the pull request -- otherwise my entire team just got laid off yesterday so our need to patch this has suddenly become less critical.

Copy link
Copy Markdown
Member

@sureshjoshi sureshjoshi left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. We've just branched for 2.32.x, so merging this pull request now will come out in 2.33.x, please move the release notes updates to docs/notes/2.33.x.md if that's appropriate.

@jackjennings
Copy link
Copy Markdown
Contributor Author

@sureshjoshi please see my comment above; if there's value here I'd like to hand this off to the team to land. Our company has been effectively dissolved so I personally don't have immediate need / ability to make the required changes.

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.

4 participants