Skip to content

Conversation

@kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented Oct 21, 2025

I was seeing failures on CI from library names getting too long while linking.

Merging this PR will:

  • Add an option (enabled by default) to the link command and babel plugin to strip the scope (@my-org in @my-org/my-pkg) from package names.

This is a breaking change if you're manually calling requireNodeAddon,

@kraenhansen kraenhansen self-assigned this Oct 21, 2025
@kraenhansen kraenhansen added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) labels Oct 21, 2025
@kraenhansen kraenhansen requested a review from Copilot October 21, 2025 08:49
Copy link
Contributor

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 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 packageName as a separate naming strategy option alongside the existing pathSuffix option
  • Renames PathSuffixChoice to LibraryNamingChoice to 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

if (rest.length === 0) {
return first;
} else {
return rest.join("-");
Copy link

Copilot AI Oct 21, 2025

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.

Suggested change
return rest.join("-");
return escapePath(rest.join("/"));

Copilot uses AI. Check for mistakes.
Comment on lines 212 to 218
return (
packageName
// Delete leading @
.replace(/^@/, "")
// Replace slashes with double-dashes
.replace(/\//g, "--")
);
Copy link

Copilot AI Oct 21, 2025

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.

Copilot uses AI. Check for mistakes.
if (transformedRelativePath) {
parts.push(transformedRelativePath);
}
return escapePath(parts.join("--"));
Copy link

Copilot AI Oct 21, 2025

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.

Suggested change
return escapePath(parts.join("--"));
return parts.map(escapePath).join("--");

Copilot uses AI. Check for mistakes.
@kraenhansen kraenhansen force-pushed the kh/package-name-stripping branch from b685e07 to 3126195 Compare October 21, 2025 09:17
@kraenhansen kraenhansen requested a review from Copilot October 21, 2025 09:20
Copy link
Contributor

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

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, "-")
Copy link

Copilot AI Oct 21, 2025

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.

Suggested change
.replace(/[^a-zA-Z0-9-_]/g, "-")
.replace(/[^a-zA-Z0-9-_]/g, "-")
// Collapse consecutive dashes into a single dash
.replace(/-+/g, "-")

Copilot uses AI. Check for mistakes.
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)
Copy link

Copilot AI Oct 21, 2025

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'.

Suggested change
// 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)

Copilot uses AI. Check for mistakes.
if (strategy === "omit") {
return "";
} else if (packageName.startsWith("@")) {
const [first, ...rest] = packageName.split("/");
Copy link

Copilot AI Oct 21, 2025

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.

Suggested change
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".`
);
}

Copilot uses AI. Check for mistakes.
@kraenhansen kraenhansen merged commit 5016ed2 into main Oct 21, 2025
6 checks passed
@kraenhansen kraenhansen deleted the kh/package-name-stripping branch October 21, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants