fix(core): improve packages recognition when the package version is an external package

[newsiberian]

Current Behavior

After updating from [email protected] to [email protected] our docusaurus dep starts blocking us from running any nx command because of the error described in the linked issue

The "nx/js/dependencies-and-lockfile" plugin threw an error while creating dependencies: Target project does not exist: npm:react-helmet-async@npm:@slorber/react-helmet-async@*

Expected Behavior

Packages with such deps, like: "react-helmet-async": "npm:@slorber/react-helmet-async@*" shouldn't break any nx command

Related Issue(s)

Fixes #27285

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Updated (UTC)
nx-dev	⬜️ Ignored (Inspect)	Visit Preview	Feb 22, 2025 11:51am

[nx-cloud]

View your CI Pipeline Execution ↗ for commit 69027a1.

Command	Status	Duration	Result
nx affected --targets=lint,test,build,e2e,e2e-c...	✅ Succeeded	34m 47s	View ↗
nx run-many -t check-imports check-commit check...	✅ Succeeded	15s	View ↗
nx-cloud record -- nx-cloud conformance:check	✅ Succeeded	2s	View ↗
nx-cloud record -- nx format:check --base= --he...	✅ Succeeded	7s	View ↗
nx-cloud record -- nx sync:check	✅ Succeeded	5s	View ↗
nx documentation	✅ Succeeded	56s	View ↗

---

☁️ Nx Cloud last updated this comment at 2025-02-24 15:12:58 UTC

[newsiberian]

@Cammisuli can this PR be reviewed, please?

[Cammisuli]

I don't believe having the npm specific checks in the project graph builder is the best place for this. The reason for this is that the builder itself should be technology/package agnostic. The project graph will contain much more types of external nodes than just npm. For example, there could be cargo, gradle, pip, etc.

We should fix the area that is actually adding the external nodes which would be in the js plugin here:

nx/packages/nx/src/plugins/js/index.ts

Lines 63 to 80 in 21a1f4e

	if (!lockFileNeedsReprocessing(lockFileHash)) {
	const nodes = readCachedParsedLockFile().externalNodes;
	parsedLockFile.externalNodes = nodes;
	return {
	externalNodes: nodes,
	};
	}
	const externalNodes = getLockFileNodes(
	packageManager,
	lockFileContents,
	lockFileHash,
	context
	);
	parsedLockFile.externalNodes = externalNodes;
	return {
	externalNodes,
	};

[newsiberian]

@Cammisuli , @meeroslav , hi, please, point me in a write direction. From what I understand the issue happens because of this line, which overrides the map key, so that key npm:react-helmet-async@npm:@slorber/react-helmet-async@* bacome npm:react-helmet-async and can't be found. At least by yarn classic and yarn 4 (basic setup).

My idea was to make a copy of external node (which has an "npm:" prefix in version) which leads to increase the number of external notes. In case of our app from 3271 to 3277. The final projectGraphDependencies isn't been changed (10400 records). But from what I understand the externalNodes is kind off dictionary map and it's not so painfully to increase the number of records within.

And my question wdyt about such solution. If I'm going in a wrong direction, please point me.

Subject: [PATCH] fix(core): improve packages recognition when version is and external
---
Index: nx-fork/packages/nx/src/plugins/js/lock-file/yarn-parser.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/nx-fork/packages/nx/src/plugins/js/lock-file/yarn-parser.ts b/nx-fork/packages/nx/src/plugins/js/lock-file/yarn-parser.ts
--- a/nx-fork/packages/nx/src/plugins/js/lock-file/yarn-parser.ts	(revision cb4a93e87217a99244160063b8993dc65d977716)
+++ b/nx-fork/packages/nx/src/plugins/js/lock-file/yarn-parser.ts	(date 1739214631533)
@@ -176,7 +176,12 @@
   for (const [packageName, versionMap] of nodes.entries()) {
     const hoistedNode = findHoistedNode(packageName, versionMap, combinedDeps);
     if (hoistedNode) {
-      hoistedNode.name = `npm:${packageName}`;
+      if (hoistedNode.data.version.startsWith('npm:')) {
+        const name = <const>`npm:${packageName}`;
+        versionMap.set(name, { ...hoistedNode, name });
+      } else {
+        hoistedNode.name = `npm:${packageName}`;
+      }
     }
 
     versionMap.forEach((node) => {

[Cammisuli]

@newsiberian did you look at modifying the findVersion function here?

nx/packages/nx/src/plugins/js/lock-file/yarn-parser.ts

Line 134 in b265192

const version = findVersion(packageName, keysArray[0], snapshot, isBerry);

You can probably do additional checks in there to handle the npm: versioning there.

[newsiberian]

You can probably do additional checks in there to handle the npm: versioning there.

thanks. Yes, I did some tweaks to allow parent func to know whether the node has an alias or not.

The important thing to note is that this code:

    if (hoistedNode) {
      hoistedNode.name = `npm:${packageName}`;
    }

actually affects on that map:

// we use key => node map to avoid duplicate work when parsing keys
let keyMap = new Map<string, ProjectGraphExternalNode>();

because they share the same nodes and first code, actually mutates the shared node. Which was quite unsuspected.

[Cammisuli]

@newsiberian awesome! It looks good on my side. Rebase the PR to get the latest things from master, and then we can get this in. Thank you very much!

[newsiberian]

@Cammisuli perfect! I think, I'd like to check a bit later whether the case must also be covered for npm/pnpm

[newsiberian]

@Cammisuli it looks like npm, pnpm are not affected. so no additional changes from my side. Synced w/ master, though

[github-actions]

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.