Allow participating buildpacks to opt-out of Node.js build scripts #928
Conversation
The buildpacks that performs installation for each of the supported package managers (npm, pnpm, Yarn) will execute a preset list of scripts from `package.json` if present: - `heroku-prebuild` - `heroku-build` or `build` - `heroku-postbuild` This PR adds a new Buildpack Plan called `node_build_scripts` to the package manager installation buildpacks that can be configured by later buildpacks that require it with the following: ```toml [[requires]] name = "node_build_scripts" [requires.metadata] enabled = <bool> ```
The buildpacks that performs installation for each of the supported package managers (npm, pnpm, Yarn) will execute a preset list of scripts from `package.json` if present: - `heroku-prebuild` - `heroku-build` or `build` - `heroku-postbuild` This PR adds a new Buildpack Plan called `node_build_scripts` to the package manager installation buildpacks that can be configured by later buildpacks that require it with the following: ```toml [[requires]] name = "node_build_scripts" [requires.metadata] enabled = <bool> ```
| .entries | ||
| .iter() | ||
| .filter(|entry| entry.name == NODE_BUILD_SCRIPTS_BUILD_PLAN_NAME) | ||
| .try_fold( |
There was a problem hiding this comment.
If two different buildpacks require node_build_scripts, one has enabled = true, and the other has enabled = false, should the scripts run? With this implementation, I think it would depend on the ordering of plan entries? I presume that would match the buildpack order, and the last buildpack would take precedence?
There was a problem hiding this comment.
That's what it looks like to me Josh, last one wins.
I wonder if always allowing a enabled = true to win would be a better behavior. Since:
- the default is to be enabled
- typically
enabled = falsewould be the special override config - then something explicitly setting
enabled = truewould be to ensure it happens even though something else disabled it.
There was a problem hiding this comment.
Could also throw an error if we find mixed true and false values, since it's sort of contradictory / non-deterministic.
Preferring enabled = true is fine with me too.
There was a problem hiding this comment.
Regarding
I wonder if always allowing a enabled = true to win would be a better behavior.
and
Could also throw an error if we find mixed true and false values
I'm not sure I understand how you both are thinking about the build plan. The entries are ordered so information given by later buildpacks should be given priority. If multiple buildpacks end up using the node_build_scripts build plan and set the enabled value then the last should win. This is not unlike how we treat environment variables. I see no reason why we should be concerned with conflicting values here.
The setting is also modelled as an Option<bool> to indicate if this value was even set at all in any of the matching build plan entries. This will help distinguish between:
- Some(true) - a participating buildpack is explicitly requesting that scripts be executed
- Some(false) - a participating buildpack is explicitly requesting that scripts should not be executed
- None - the default behavior is used
| .entries | ||
| .iter() | ||
| .filter(|entry| entry.name == NODE_BUILD_SCRIPTS_BUILD_PLAN_NAME) | ||
| .try_fold( |
There was a problem hiding this comment.
That's what it looks like to me Josh, last one wins.
I wonder if always allowing a enabled = true to win would be a better behavior. Since:
- the default is to be enabled
- typically
enabled = falsewould be the special override config - then something explicitly setting
enabled = truewould be to ensure it happens even though something else disabled it.
There was a problem hiding this comment.
My understanding is that this is here to prevent double builds, when other processes run npm build inside of the resulting container. I think I actually prefer the double build scenario -- the npm build artifacts end up as part of the image, which means it will run as customers expect without having to use volumes or run another process.
I don't have any issue with the API, really, other than the non-determinism mentioned in my other comment.
Not necessarily… for Static Web Apps, this can mean that the image defaults to serving the wrong or broken version of the site. Ideally with release-build configured, artifacts will only be generated by release-build. |
The buildpacks that performs installation for each of the supported package managers (npm, pnpm, Yarn) will execute a preset list of scripts from
package.jsonif present:heroku-prebuildheroku-buildorbuildheroku-postbuildThis PR adds a new Buildpack Plan called
node_build_scriptsto these package manager installation buildpacks that can be configured by later buildpacks that require it. For example, the scripts can be prevented from running with the following: