Add npm to nodejs-engine build plan

[colincasey]

A default version of npm comes bundled with every Node.js installation so this is now indicated in the build plan.

W-13916400

[schneems]

I understand the high-level truthfulness that this buildpack provides npm, but I don't understand the specific use case of when this is intended to be used. I'm comparing this to the Ruby buildpack which only currently provides ruby but it also installs bundler, as well as running rake assets compilation, but if you only wanted bundler and not Ruby I think you would be surprised to requires("bundler") and get a Ruby version installed too. I think the same might be true of the node buildpack.

I see these provides/requires values more as tags to specific buildpacks than as actual generic features. I could use some help understanding either an app user or buildpack maintainer's perspective on how this is intended to be used versus what's available today.

[colincasey]

in regards to your questions @schneems:

I understand the high-level truthfulness that this buildpack provides npm, but I don't understand the specific use case of when this is intended to be used. I'm comparing this to the Ruby buildpack which only currently provides ruby but it also installs bundler, as well as running rake assets compilation, but if you only wanted bundler and not Ruby I think you would be surprised to requires("bundler") and get a Ruby version installed too. I think the same might be true of the node buildpack.

the node-engine buildpack cannot install dependencies like the ruby buildpack might because we support multiple package managers (i.e.; npm, yarn, pnpm). so node-engine gets combined with a matching package manager buildpack that runs after. still, the npm tooling is provided by node-engine because it comes with every install of node which is why i thought it would helpful to specify that.

the way i intended it to be used was in conjunction with #623 and #625 so that the nodejs meta-buildpack would have eventually have the following group defined:

• node-engine (provides node + npm)
• node-npm-engine (optional, requires node, provides npm)
• node-npm-install (requires node + npm + node_modules, provides node_modules)

that would support a standard npm-based application that either:

• has no specific version of npm declared (only node-engine and node-npm-install are active)
• has a specific version of npm declared in engines.npm (all 3 three buildpacks are active)

the strange this about this PR is that providing npm here fails the integration test with the following:

===> DETECTING
======== Results ========
pass: heroku/nodejs-engine@1.1.4
Resolving plan... (try #1)
fail: heroku/nodejs-engine@1.1.4 provides unused npm
ERROR: No buildpack groups passed detection.
ERROR: Please check that you are running against the correct path.
ERROR: failed to detect: no buildpacks participating

which seems surprising. i would think that if a buildpack provides something that is not required by later buildpacks but all other require criteria are met then this should be fine but, from the above error, this doesn't appear to be the case. it seems like this would need to be combined with #623 and #625 to work fully.

[edmorley]

the strange this about this PR is that providing npm here fails the integration test with the following:

This is part of the spec:
https://github.com/buildpacks/spec/blob/main/buildpack.md#phase-1-detection

If a required buildpack provides a dependency that is not required by the same buildpack or a subsequent buildpack, the trial MUST fail to detect.

The solution is to have an OR and say ~"(provides node and npm) OR (provides node)"

[colincasey]

thanks, @edmorley. i figured i was misinterpreting what the spec said about the build plan. i didn't realize the implication of the [[or]] declarations.

[joshwlewis]

The solution is to have an OR and say ~"(provides node and npm) OR (provides node)"

I think there is another solution. Have this buildpack also require npm.

I don't love the OR idea - it seems misleading to say this buildpack provides npm only some of the time. This buildpack always installs npm if it runs, so always put it in the provides seems closer to developer expectations.

[edmorley]

@joshwlewis I don't think it's misleading per-se, it's just how the build plan is meant to be used IMO.

ie: Let's say I have some other buildpack that only needs Node and doesn't need NPM (eg it's running a JS script that has no dependencies). Without the or, this other buildpack would have to incorrectly state that it required both node and npm, otherwise detection would always fail.

That is, I think "provides" is meant to be more of a "here are the various scenarios I cater for, pick and match".

[joshwlewis]

ie: Let's say I have some other buildpack that only needs Node and doesn't need NPM (eg it's running a JS script that has no dependencies). Without the or, this other buildpack would have to incorrectly state that it required both node and npm, otherwise detection would always fail.

Having this buildpack self-require npm (instead of using OR) would allow downstream buildpacks to require node without npm too.

That is, I think "provides" is meant to be more of a "here are the various scenarios I cater for, pick and match".

Yeah, I think folks who know the CNB spec well understand this. My comment about developer expectations was targeted at someone less familiar (like someone writing a JS script buildpack).

[edmorley]

Having this buildpack self-require npm (instead of using OR) would allow downstream buildpacks to require node without npm too.

Ah true :-)

[colincasey]

I think there is another solution. Have this buildpack also require npm.

i also find this usage to be as confusing as the or usage. why claim this buildpack requires npm when it doesn't?

I don't love the OR idea - it seems misleading to say this buildpack provides npm only some of the time. This buildpack always installs npm if it runs, so always put it in the provides seems closer to developer expectations.

i'm glad i'm not the only one who finds this contrary to developer expectations.

it's just how the build plan is meant to be used IMO.

i'm not sure why an unmet provides should cause an issue though. i get why requires would but if something is provided but not used, why does that matter to the build? if this is how the build plan is meant to be interpreted then fine, but it seems like an unnecessary restriction to me.

[edmorley]

@joshwlewis @colincasey What is the status of / next steps for this PR? Both the PR and the associated GUS work item have been in the "awaiting review" state for 2 weeks.