Skip to content

Conversation

nathanwhit
Copy link
Member

Fixes #28933.

This was a regression from the packument refactor.

@nathanwhit nathanwhit requested review from dsherret and Copilot April 21, 2025 17:49
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a regression from the packument refactor by ensuring deprecation warnings are printed only on the first install. Key changes include:

  • Adding a new test case for deprecation warnings in the npm specs.
  • Removing the helper function handle_package_scripts_bin_deprecated and inlining its behavior.
  • Updating conditionals to control when deprecation warnings are recorded.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
tests/specs/npm/deprecation_warnings_no_repeat/main.ts Adds a test to verify that deprecation warnings are not repeated on subsequent installs
cli/npm/installer/local.rs Inlines handling of package scripts, bin setups, and deprecation warnings, with modified conditional logic
Files not reviewed (1)
  • tests/specs/npm/deprecation_warnings_no_repeat/test.jsonc: Language not supported
Comments suppressed due to low confidence (1)

cli/npm/installer/local.rs:451

  • Removing the check for package.is_deprecated in this condition might cause deprecated packages without bin or scripts to be skipped from deprecation warnings. Confirm that this is the intended behavior for first install deprecation handling.
if package.has_bin || package.has_scripts {

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. LGTM

@nathanwhit nathanwhit enabled auto-merge (squash) April 21, 2025 18:34
@nathanwhit nathanwhit merged commit 713bf32 into denoland:main Apr 21, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excessive npm warnings
2 participants