Skip to content

Commit

Permalink
Fix warning for checked in node_modules dir
Browse files Browse the repository at this point in the history
One of the features of the `commons` logger that was removed in #993 was the ability to collect log messages that would be emitted at a later point. This feature is not available in `bullet_stream` so when I moved the logging message that notifies the user they've checked in their `node_modules` directory to the end of the build phase, I mistakenly moved the timing of that check to run at the end of the build phase as well.

This check needs to run at the beginning of the build phase and the warning needs to be emitted at the end. This PR contains changes to do this + adds an integration test to verify this case.
  • Loading branch information
colincasey committed Jan 9, 2025
1 parent 92bffcb commit 0d4556b
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 6 deletions.
4 changes: 4 additions & 0 deletions buildpacks/nodejs-npm-install/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- Warning for checked in `node_modules` di

## [3.4.2] - 2025-01-08

- No changes.
Expand Down
13 changes: 7 additions & 6 deletions buildpacks/nodejs-npm-install/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ impl Buildpack for NpmInstallBuildpack {
let node_build_scripts_metadata = read_node_build_scripts_metadata(&context.buildpack_plan)
.map_err(NpmInstallBuildpackError::NodeBuildScriptsMetadata)?;

let prebuild_modules_warning = application::warn_prebuilt_modules(app_dir);

application::check_for_singular_lockfile(app_dir)
.map_err(NpmInstallBuildpackError::Application)?;

Expand All @@ -98,12 +100,11 @@ impl Buildpack for NpmInstallBuildpack {

configure_npm_runtime_env(&context)?;

let logger =
if let Some(prebuilt_modules_warning) = application::warn_prebuilt_modules(app_dir) {
logger.warning(prebuilt_modules_warning)
} else {
logger
};
let logger = if let Some(warning_message) = prebuild_modules_warning {
logger.warning(warning_message)
} else {
logger
};

logger.done();
build_result
Expand Down
23 changes: 23 additions & 0 deletions buildpacks/nodejs-npm-install/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,29 @@ fn test_default_web_process_registration_is_skipped_if_procfile_exists() {
);
}

#[test]
#[ignore]
fn npm_modules_checked_in_warning() {
nodejs_integration_test("./fixtures/npm-project", |ctx| {
assert_not_contains!(
ctx.pack_stdout,
"Warning: `node_modules` checked into source control"
);

let mut config = ctx.config.clone();
config.app_dir_preprocessor(|app_dir| {
std::fs::create_dir(app_dir.join("node_modules")).unwrap();
});

ctx.rebuild(config, |ctx| {
assert_contains!(
ctx.pack_stdout,
"Warning: `node_modules` checked into source control"
);
});
});
}

fn add_lockfile_entry(app_dir: &Path, package_name: &str, lockfile_entry: serde_json::Value) {
update_json_file(&app_dir.join("package-lock.json"), |json| {
let dependencies = json["dependencies"].as_object_mut().unwrap();
Expand Down

0 comments on commit 0d4556b

Please sign in to comment.