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

javascript: Add an explicit npm_distribution target instead of coupling to node_package #18925

Merged

Conversation

tobni
Copy link
Contributor

@tobni tobni commented May 6, 2023

The coupling of node_package (generated from the package_json target) with the package goal caused some unwanted side effects when trying to integrate with other backends, docker to be exact.

Because of the way the dependency graph is constructed between in-repo nodejs packages, this caused package:able targets to always be included.
See this bit of code:

embedded_pkgs_per_target_request = Get(

The transitive dependencies of the docker_image currently causes packaging of workspace packages, even though you probably do not want to include the resulting tarballs at all.

Even worse, some package manager + workspace combinations are flat out broken, see pnpm/pnpm#4351.

This solution "moves" the package:able target out of the dependency graph by placing it as standalone thing, outside of the automagically constructed dependencies.

Fixes #18922
This PR does not provide a work-around for the linked PNPM issue.

@stuhood stuhood requested a review from kaos May 8, 2023 21:23
@stuhood
Copy link
Member

stuhood commented May 8, 2023

@kaos : Thoughts on whether the docker backend's dependencies should be transitive like this?

@tobni : Independent of the issue of transitivity in consumers, do you think that you still prefer this interface? Or would it make more sense to try and change the behavior of the consumer?

@tobni
Copy link
Contributor Author

tobni commented May 8, 2023

I do prefer this interface change regardless.

@tobni tobni force-pushed the add/implement-a-explicit-npm-distribution-target branch from 882b004 to 919abae Compare May 9, 2023 17:19
@stuhood stuhood enabled auto-merge (squash) May 9, 2023 18:08
@stuhood
Copy link
Member

stuhood commented May 9, 2023

@tobni : For future reference:

Addresses #18922

If you change this to "Fixes #18922" or "Closes #18922", the mentioned ticket will close automatically when the PR does.

@tobni tobni force-pushed the add/implement-a-explicit-npm-distribution-target branch from 919abae to d753b54 Compare May 12, 2023 18:31
@kaos
Copy link
Member

kaos commented May 15, 2023

@kaos : Thoughts on whether the docker backend's dependencies should be transitive like this?

@stuhood sorry, not following what you are referring to as different in the Docker backend?

Edit: ah, I looked at the code, now I see the disussion in the PR description, reading up on that...

Edit2: right, think I get it now. Meaning, that perhaps the docker backend should only work on direct dependencies, not transitive, which would've also avoided this issue. I think that could make sense, but would rule out the use of things like grouping a set of packageables using target for instance in order to reduce BUILD file duplications. But that's a rather hypothetical argument..

auto-merge was automatically disabled May 16, 2023 15:07

Head branch was pushed to by a user without write access

@stuhood stuhood enabled auto-merge (squash) May 16, 2023 16:32
auto-merge was automatically disabled May 16, 2023 20:13

Head branch was pushed to by a user without write access

@stuhood stuhood enabled auto-merge (squash) May 16, 2023 20:29
@stuhood stuhood merged commit fbc53f0 into pantsbuild:main May 16, 2023
@tobni tobni deleted the add/implement-a-explicit-npm-distribution-target branch May 19, 2023 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

javascript: Add explicit npm_distribution target
3 participants