- 
                Notifications
    
You must be signed in to change notification settings  - Fork 6
 
Add package name stripping the "scope" part from library names #277
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
Conversation
There was a problem hiding this 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 adds a new option to control how package names are transformed into library names during the linking process, specifically addressing issues with excessively long library names during linking by allowing scopes to be stripped from package names.
Key changes:
- Introduces 
packageNameas a separate naming strategy option alongside the existingpathSuffixoption - Renames 
PathSuffixChoicetoLibraryNamingChoiceto reflect its broader usage - Refactors library name generation logic to handle both package name and path transformations independently
 
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description | 
|---|---|
| packages/host/src/node/path-utils.ts | Adds packageName strategy field, renames types to LibraryNamingChoice, extracts transformation logic into separate functions | 
| packages/host/src/node/path-utils.test.ts | Adds test cases for scoped package name transformations with different strategies | 
| packages/host/src/node/cli/program.ts | Integrates packageName option into CLI commands (link, list, module) | 
| packages/host/src/node/cli/options.ts | Adds packageNameOption CLI option with environment variable support | 
| packages/host/src/node/babel-plugin/plugin.ts | Updates Babel plugin to accept and use packageName option | 
| apps/test-app/babel.config.js | Updates example comment to show both packageName and pathSuffix options | 
| .changeset/orange-bananas-obey.md | Documents the breaking change in changeset | 
        
          
                packages/host/src/node/path-utils.ts
              
                Outdated
          
        
      | if (rest.length === 0) { | ||
| return first; | ||
| } else { | ||
| return rest.join("-"); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 21, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When stripping the scope, multiple path segments after the first are joined with single dashes, but this could conflict with the double-dash separator used elsewhere. For example, @org/pkg/subpath would become pkg-subpath which could be ambiguous. Consider using rest.join("/") and letting escapePath handle the transformation consistently, or document this behavior.
| return rest.join("-"); | |
| return escapePath(rest.join("/")); | 
        
          
                packages/host/src/node/path-utils.ts
              
                Outdated
          
        
      | return ( | ||
| packageName | ||
| // Delete leading @ | ||
| .replace(/^@/, "") | ||
| // Replace slashes with double-dashes | ||
| .replace(/\//g, "--") | ||
| ); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 21, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transformation logic for 'keep' strategy uses double-dashes for slashes but doesn't pass through escapePath. This creates inconsistency since escapePath is applied to the final joined result at line 259. Consider whether package names should be escaped before transformation, or document why double-dash replacement is sufficient for package name slashes.
        
          
                packages/host/src/node/path-utils.ts
              
                Outdated
          
        
      | if (transformedRelativePath) { | ||
| parts.push(transformedRelativePath); | ||
| } | ||
| return escapePath(parts.join("--")); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 21, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applying escapePath after joining with '--' will convert the double-dash separators to single dashes (since escapePath replaces all non-alphanumeric characters). This breaks the intended double-dash convention for separating package name and path components. The escapePath call should be applied to individual parts before joining.
| return escapePath(parts.join("--")); | |
| return parts.map(escapePath).join("--"); | 
…tion defaulting to strip
b685e07    to
    3126195      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
| return ( | ||
| modulePath | ||
| // Replace any non-alphanumeric character with a dash | ||
| .replace(/[^a-zA-Z0-9-_]/g, "-") | 
    
      
    
      Copilot
AI
    
    
    
      Oct 21, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex now preserves underscores and hyphens, but consecutive dashes could result from multiple non-alphanumeric characters. Consider collapsing consecutive dashes with .replace(/-+/g, '-') after the first replacement.
| .replace(/[^a-zA-Z0-9-_]/g, "-") | |
| .replace(/[^a-zA-Z0-9-_]/g, "-") | |
| // Collapse consecutive dashes into a single dash | |
| .replace(/-+/g, "-") | 
        
          
                packages/host/src/node/path-utils.ts
              
                Outdated
          
        
      | if (strategy === "strip") { | ||
| return escapePath(rest.join("/")); | ||
| } else { | ||
| // Stripping away the @ and using double underscore to separate scope and name is common practice other projects (like DefinitelyTyped) | 
    
      
    
      Copilot
AI
    
    
    
      Oct 21, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing 'in' between 'practice' and 'other'. Should read 'common practice in other projects'.
| // Stripping away the @ and using double underscore to separate scope and name is common practice other projects (like DefinitelyTyped) | |
| // Stripping away the @ and using double underscore to separate scope and name is common practice in other projects (like DefinitelyTyped) | 
| if (strategy === "omit") { | ||
| return ""; | ||
| } else if (packageName.startsWith("@")) { | ||
| const [first, ...rest] = packageName.split("/"); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 21, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a scoped package name lacks a slash (e.g., just @scope), rest will be empty and rest.join('/') returns an empty string. This edge case should be handled or validated.
| const [first, ...rest] = packageName.split("/"); | |
| const [first, ...rest] = packageName.split("/"); | |
| if (rest.length === 0) { | |
| throw new Error( | |
| `Invalid scoped package name "${packageName}". Expected format "@scope/name".` | |
| ); | |
| } | 
I was seeing failures on CI from library names getting too long while linking.
Merging this PR will:
@my-orgin@my-org/my-pkg) from package names.This is a breaking change if you're manually calling
requireNodeAddon,