Skip to content

Commit

Permalink
Remove check and warning for node_modules directory (#998)
Browse files Browse the repository at this point in the history
This warning was carried over from the classic Heroku Node.js buildpack which did support both `npm install` and `npm ci`. If a `node_modules` directory existed and there was no lockfile present, this could cause issues with the dependencies installed.

This buildpack only supports `npm ci` so this check and warning is not required. If the `node_modules` directory is already present, `npm ci` will automatically remove it.
  • Loading branch information
colincasey authored Jan 10, 2025
1 parent 92bffcb commit 6a98a4f
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 26 deletions.
5 changes: 5 additions & 0 deletions buildpacks/nodejs-npm-install/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Removed

- Check and warning for `node_modules` directory is unnecessary since
`npm ci` will remove it before installing dependencies. ([#998]https://github.com/heroku/buildpacks-nodejs/pull/998))

## [3.4.2] - 2025-01-08

- No changes.
Expand Down
7 changes: 0 additions & 7 deletions buildpacks/nodejs-npm-install/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,6 @@ 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
};

logger.done();
build_result
}
Expand Down
20 changes: 2 additions & 18 deletions common/nodejs-utils/src/application.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::package_manager::PackageManager;
use bullet_stream::style;
use indoc::{formatdoc, writedoc};
use indoc::writedoc;
use std::fmt::{Display, Formatter};
use std::path::Path;

Expand Down Expand Up @@ -28,23 +28,6 @@ pub fn check_for_singular_lockfile(app_dir: &Path) -> Result<(), Error> {
}
}

/// Checks if the `node_modules` folder is present in the given directory which indicates that
/// the application contains files that it shouldn't in its git repository. If this is the case,
/// a warning message will be returned.
#[must_use]
pub fn warn_prebuilt_modules(app_dir: &Path) -> Option<String> {
if app_dir.join("node_modules").exists() {
Some(formatdoc! {"
Warning: {node_modules} checked into source control
Add these files and directories to {gitignore}. See the Dev Center for more info:
https://devcenter.heroku.com/articles/node-best-practices#only-git-the-important-bits
", node_modules = style::value("node_modules"), gitignore = style::value(".gitignore") })
} else {
None
}
}

#[derive(Debug)]
pub enum Error {
MissingLockfile,
Expand Down Expand Up @@ -132,6 +115,7 @@ impl Display for Error {
#[cfg(test)]
mod tests {
use super::*;
use indoc::formatdoc;

#[test]
fn test_error_output_for_multiple_lockfiles() {
Expand Down
2 changes: 1 addition & 1 deletion common/nodejs-utils/src/package_json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl PackageJson {
pub fn has_start_script(&self) -> bool {
self.scripts
.as_ref()
.map_or(false, |scripts| scripts.start.is_some())
.is_some_and(|scripts| scripts.start.is_some())
}
}

Expand Down

0 comments on commit 6a98a4f

Please sign in to comment.