Skip to content

PackageLockJsonTest update to allow for transitive dependencies with "npm:" alias prefix#2995

Merged
cnathe merged 8 commits into
developfrom
fb_packageLockTestUpdate
May 13, 2026
Merged

PackageLockJsonTest update to allow for transitive dependencies with "npm:" alias prefix#2995
cnathe merged 8 commits into
developfrom
fb_packageLockTestUpdate

Conversation

@cnathe
Copy link
Copy Markdown
Contributor

@cnathe cnathe commented May 13, 2026

Rationale

This PR updates PackageLockJsonTest to allow transitive dependency version strings that use npm's package alias syntax (npm:<package>@<range>). The original approach used a hardcoded allowlist of three specific aliases from
@isaacs/cliui; the new approach allows any version string starting with npm:, which is more general and won't need updating when new npm aliases appear in lock files.

Changes

  • update to allow for transitive dependencies with "npm:" alias prefix

@cnathe cnathe changed the title PackageLockJsonTest update to add "npm:react-is@<versions>" aliases to ALLOWED_NONSTANDARD_VERSIONS PackageLockJsonTest update to allow for transitive dependencies with "npm:" alias prefix May 13, 2026
@cnathe cnathe marked this pull request as ready for review May 13, 2026 14:39
@cnathe cnathe self-assigned this May 13, 2026
{
String tVer = transitiveDeps.optString(tDep);
if (tVer == null || tVer.contains(":") && !ALLOWED_NONSTANDARD_VERSIONS.contains(tVer)) // URL, file, or workspace dependency
if (tVer == null || (tVer.contains(":") && !tVer.startsWith("npm:"))) // URL, file, or workspace dependency
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure it really matters but maybe make this trim off the "npm:" instead of just skipping any versions starting with it.

Suggested change
if (tVer == null || (tVer.contains(":") && !tVer.startsWith("npm:"))) // URL, file, or workspace dependency
if (tVer == null || tVer.replaceAll("^npm:", "").contains(":")) // URL, file, or workspace dependency

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

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

I piggy-backed some logging improvements but this change looks ok to me.

@cnathe cnathe merged commit 80112df into develop May 13, 2026
7 of 8 checks passed
@cnathe cnathe deleted the fb_packageLockTestUpdate branch May 13, 2026 21:58
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.

3 participants