Skip to content

fix: local transitive dependencies with --install-links=true #8436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: latest
Choose a base branch
from

Conversation

owlstronaut
Copy link
Contributor

@owlstronaut owlstronaut commented Jul 15, 2025

Fix: Local transitive dependencies with --install-links=true

Problem Statement

Issue #5733: npm install --install-links=true fails when projects have local transitive dependencies (file dependencies that reference other packages within the same project root).

Root Cause: The dependency resolution logic in build-ideal-tree.js treated all file: dependencies identically, without distinguishing between project-internal dependencies (which should always be symlinked) and external file dependencies (which should respect the installLinks setting).

Solution

This PR adds logic to detect project-internal file dependencies and ensures they are always symlinked, regardless of the installLinks setting, while external file dependencies continue to respect the user's preference.

Key Changes

File: workspaces/arborist/lib/arborist/build-ideal-tree.js

// Detect project-internal file dependencies
let isProjectInternalFileSpec = false
if (edge && edge.rawSpec && edge.rawSpec.startsWith('file:') &&
    (edge.rawSpec.startsWith('file:../') || edge.rawSpec.startsWith('file:./'))) {
  const { resolve, sep } = require('path')
  const parentPath = parent.realpath || parent.path
  const targetPath = resolve(parentPath, edge.rawSpec.replace(/^file:/, ''))
  const projectRoot = this.idealTree.realpath || this.idealTree.path
  const resolvedProjectRoot = resolve(projectRoot)
  isProjectInternalFileSpec = targetPath.startsWith(resolvedProjectRoot + sep) || targetPath === resolvedProjectRoot
}

const shouldLink = isWorkspace || isProjectInternalFileSpec || (!isProjectInternalFileSpec && !installLinks)
if (spec.type === 'directory' && shouldLink) {
  return this.#linkFromSpec(name, spec, parent, edge)
}

Behavior Matrix

Dependency Type installLinks=true installLinks=false
Workspace LINK LINK
Project-internal file (file:./..., file:../... within project) LINK LINK
External file (file:../... outside project) COPY LINK
Registry package COPY LINK

Comparison with Previous Fix Attempt

Technical Strategy Previous Attempt This Fix
Problem Analysis Treated as edge case in dependency resolution flow Recognized as fundamental distinction between internal vs external file deps
Solution Scope Modified multiple components across the dependency pipeline Single decision point enhancement in existing link/copy logic
Path Handling String manipulation and pattern matching on spec paths Canonical path resolution using Node.js path module
Logic Integration Added new conditional branches throughout resolution process Extended existing conditional logic with precise path-based detection
Dependency Classification Attempted to special-case file dependencies at resolution time Introduced semantic distinction between project-internal and external file deps

Technical Details

Detection Logic: A file dependency is considered project-internal if its resolved target path is within or equal to the project root directory.

Integration Point: The fix integrates cleanly into the existing #nodeFromSpec() method's link vs copy decision logic without requiring changes to other parts of the codebase.

Risk Assessment

Low Risk: This is a targeted fix that only affects the specific broken use case. All existing functionality is preserved, and the change is isolated to a single decision point in the dependency resolution flow.

@owlstronaut owlstronaut force-pushed the owlstronaut/transitive-deps branch 3 times, most recently from e5759e7 to 89beca4 Compare July 15, 2025 22:38
@owlstronaut owlstronaut force-pushed the owlstronaut/transitive-deps branch from 89beca4 to ac962a7 Compare July 15, 2025 22:45
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.

1 participant