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

Merged
merged 1 commit into from
Jul 17, 2025

Conversation

owlstronaut
Copy link
Contributor

@owlstronaut owlstronaut commented Jul 15, 2025

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

closes #8342
closes #5733

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: When installLinks=true, the dependency resolution logic copied all file: dependencies instead of symlinking them. For project-internal dependencies, copying breaks transitive dependency resolution because the copied package loses its relative path context to other parts of the project.

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

    let isProjectInternalFileSpec = false
    if (edge && edge.rawSpec && edge.rawSpec.startsWith('file:') &&
        (edge.rawSpec.startsWith('file:../') || edge.rawSpec.startsWith('file:./'))) {
      const targetPath = resolve(parent.realpath, edge.rawSpec.replace(/^file:/, ''))
      const resolvedProjectRoot = resolve(this.idealTree.realpath)
      // Check if the target is within the project root
      isProjectInternalFileSpec = targetPath.startsWith(resolvedProjectRoot + sep) || targetPath === resolvedProjectRoot
    }
    // Decide whether to link or copy the dependency
    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 ⭐ (New Behavior) LINK
External file (file:../... outside project) COPY LINK
Registry package COPY LINK

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.

FlowChart
flowchart TD
    A[Directory Dependency Found] --> B{Is it a Workspace?}
    B -->|Yes| LINK1[🔗 LINK - Workspace]
    B -->|No| C{Is it file: dependency?}
    
    C -->|No| D{installLinks setting?}
    C -->|Yes| E{Relative path?<br/>file:../ or file:./}
    
    E -->|No| D
    E -->|Yes| F[Resolve target path]
    F --> G{Target within<br/>project root?}
    
    G -->|Yes| LINK2[🔗 LINK - Project Internal<br/>⭐ NEW BEHAVIOR]
    G -->|No| D
    
    D -->|installLinks=false| LINK3[🔗 LINK - External]
    D -->|installLinks=true| COPY[📁 COPY - External]
Loading

@owlstronaut owlstronaut force-pushed the owlstronaut/transitive-deps branch 5 times, most recently from ac962a7 to 506ecf1 Compare July 16, 2025 16:47
@owlstronaut owlstronaut marked this pull request as ready for review July 16, 2025 17:26
@owlstronaut owlstronaut requested a review from a team as a code owner July 16, 2025 17:26
@owlstronaut owlstronaut force-pushed the owlstronaut/transitive-deps branch from 506ecf1 to 3956520 Compare July 16, 2025 20:52
@owlstronaut owlstronaut requested a review from wraithgar July 17, 2025 18:36
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

Very clean, yay.

@owlstronaut owlstronaut merged commit 6dbe21a into latest Jul 17, 2025
16 checks passed
@owlstronaut owlstronaut deleted the owlstronaut/transitive-deps branch July 17, 2025 20:41
@github-actions github-actions bot mentioned this pull request Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants