refactor(node): Re-write Yarn to no more rely on `node_modules` file hierarchy #9427

fviernau · 2024-11-14T13:13:41Z

See individual commits.

Part of: #9261.

codecov · 2025-03-19T09:29:01Z

Codecov Report

Attention: Patch coverage is 84.41558% with 24 lines in your changes missing coverage. Please review.

Project coverage is 56.47%. Comparing base (e161da3) to head (2139e12).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...anagers/node/src/main/kotlin/yarn/WorkspaceInfo.kt	46.66%	5 Missing and 3 partials ⚠️
...managers/node/src/main/kotlin/yarn/YarnListNode.kt	55.55%	2 Missing and 6 partials ⚠️
...package-managers/node/src/main/kotlin/yarn/Yarn.kt	94.25%	0 Missing and 5 partials ⚠️
...node/src/main/kotlin/yarn/YarnDependencyHandler.kt	91.17%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9427      +/-   ##
============================================
- Coverage     56.49%   56.47%   -0.03%     
+ Complexity     1587     1572      -15     
============================================
  Files           327      328       +1     
  Lines         12371    12381      +10     
  Branches       1145     1162      +17     
============================================
+ Hits           6989     6992       +3     
+ Misses         4927     4926       -1     
- Partials        455      463       +8

Flag	Coverage Δ
funTest-docker	`68.70% <84.41%> (-0.14%)`	⬇️
funTest-non-docker	`33.26% <ø> (ø)`
test-ubuntu-24.04	`39.24% <5.19%> (-0.16%)`	⬇️
test-windows-2022	`39.22% <5.19%> (-0.16%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

plugins/package-managers/node/src/main/kotlin/yarn/WorkspaceInfo.kt

plugins/package-managers/node/src/main/kotlin/yarn/Yarn.kt

plugins/package-managers/node/src/main/kotlin/NodePackageManagerSupport.kt

plugins/package-managers/node/src/main/kotlin/yarn/WorkspaceInfo.kt

plugins/package-managers/node/src/main/kotlin/yarn/Yarn.kt

sschuberth · 2025-03-21T13:16:37Z

plugins/package-managers/node/src/main/kotlin/yarn/Yarn.kt

+private fun getInstalledModules(moduleDir: File) =
+    moduleDir.resolve("node_modules").walkBottomUp().filter {
+        it.isFile && it.name == "package.json"
+    }.mapTo(mutableListOf()) { parsePackageJson(it) }


Looks like all callers are fine with returning a sequence, so just drop the "To(mutableListOf())" part, keeping only "map". (Also, currently the function returns a mutable list due to the implicit return type.)

I've made the type explicit but kept it a list, as this is more handy with debugging.

plugins/package-managers/node/src/main/kotlin/yarn/Yarn.kt

plugins/package-managers/node/src/main/kotlin/yarn/YarnDependencyHandler.kt

sschuberth · 2025-03-21T13:36:00Z

plugins/package-managers/node/src/main/kotlin/yarn/YarnDependencyHandler.kt


-    override fun dependenciesFor(dependency: ModuleInfo): List<ModuleInfo> = dependency.dependencies.toList()
+    override fun linkageFor(dependency: YarnListNode): PackageLinkage = PackageLinkage.DYNAMIC


How are project dependencies, e.g. in a workspace, handled? I.e. why can this never be PackageLinkage.PROJECT_DYNAMIC compared to before?

I had forgotten to implement this. Now, it supports project references inbetween workspace projects.

Hmm, as I believe tests had passed for your first iteration, does that mean that this functionality was not covered by test before?

Hmm, as I believe tests had passed for your first iteration, does that mean that this functionality was not covered by test before?

Yes, exactly. (I put on my TODO list to add tests for that, but consistent for all node managers. I didn't do it as part of this PR as it also wasn't covered by tests before.)

Fair enough. Although I have to admit that this decreases confidence in the rewrite a bit, as it's now harder to judge whether it changes behavior compared to before or not.

Can I ask for your timeline to add these tests? Because if this refactoring broke something, then it wouldn't be so good to let it go unnoticed for too long.

Fair enough. Although I have to admit that this decreases confidence in the rewrite a bit, as it's now harder to judge whether it changes behavior compared to before or not.

Ok, then I'll make a PR prior to this one which enhance the tests.

FYI, I'm waiting for these tests before another review.

It turned out that the existing implementation wasn't capable of handling it. So, I've fixed that and covered it with a test, see #10177.

ok, now it's ready again @sschuberth .

plugins/package-managers/node/src/main/kotlin/NodePackageManagerSupport.kt

sschuberth · 2025-04-16T13:15:02Z

plugins/package-managers/node/src/main/kotlin/yarn/Yarn.kt

                packages = emptySet(),
-                issues = issues
+                issues = emptyList()


It is intended that we now never get any issues here compared to before?

resolveDependencies() is called for each (mapped) definition file, let's name it D.
We still do get an issue for each D, for which the at least one project's PackageJson which corresponds to D's workspace could not be parsed. The issue creation happens here: 1.

So, you're saying that the task of catching exceptions while parsing project files is now handed over from here to upstream PackageManager code, correct? Or in other words, your new implementation assumes that if you reach this line, there were no issues with parsing the project, right?

So, you're saying that the task of catching exceptions while parsing project files is now handed over from here to upstream PackageManager code, correct?

Yes.

Or in other words, your new implementation assumes that if you reach this line, there were no issues with parsing the project, right?

Yes.

The use of `npm view` is an implementation detail of `getRemotePackageDetails()`. While at it, use a reference to `PackageJsonFile`. Signed-off-by: Frank Viernau <[email protected]>

Signed-off-by: Frank Viernau <[email protected]>

Prepare for re-use. Signed-off-by: Frank Viernau <[email protected]>

Re-write large parts of `Yarn` to prefer extracting the needed information from the output of the `yarn` CLI command. In particular, the dependency tree is constructed now from the output of `yarn list` instead of from the file hierarchy of the `node_modules` directory. The approach is as follows: 1. Install the modules via `yarn install`. 2. Determine the workspace module dirs via `yarn workspaces list`. 3. Determine all module dirs by searching for any `package.json`. 4. Construct the scopes from `yarn list --json {--prod|--dev}`. Use the determined module dirs and workspace module dirs to decide whether a dependency is a project or a package. 5. Create project and packages by also parsing the `package.json` files in the module dirs. Note: The `YarnDependencyHandlerTest` is removed, because the handler is covered by the functional tests. This is consistent with other (Node) package manager tests. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the yarn-rewrite branch from c097366 to 9eca356 Compare November 19, 2024 11:49

fviernau mentioned this pull request Jan 24, 2025

Yarn chores #9833

Merged

fviernau force-pushed the yarn-rewrite branch 2 times, most recently from 6e4175d to 603c0d7 Compare March 19, 2025 09:14

fviernau force-pushed the yarn-rewrite branch 5 times, most recently from 55d15ca to ac202b3 Compare March 20, 2025 14:08

github-advanced-security bot found potential problems Mar 20, 2025

View reviewed changes

plugins/package-managers/node/src/main/kotlin/yarn/WorkspaceInfo.kt Fixed Show fixed Hide fixed

plugins/package-managers/node/src/main/kotlin/yarn/Yarn.kt Fixed Show fixed Hide fixed

fviernau force-pushed the yarn-rewrite branch 2 times, most recently from aeb93b3 to b787994 Compare March 20, 2025 14:53

fviernau marked this pull request as ready for review March 20, 2025 14:53

fviernau requested a review from a team as a code owner March 20, 2025 14:53

fviernau force-pushed the yarn-rewrite branch 2 times, most recently from 36effda to 961fc48 Compare March 20, 2025 15:36

sschuberth requested changes Mar 21, 2025

View reviewed changes

fviernau force-pushed the yarn-rewrite branch 3 times, most recently from 1a79a8d to e6a3fdd Compare March 24, 2025 08:42

fviernau requested a review from sschuberth March 24, 2025 08:42

fviernau force-pushed the yarn-rewrite branch from e6a3fdd to 0e9128e Compare March 24, 2025 08:47

fviernau force-pushed the yarn-rewrite branch 2 times, most recently from 9395121 to ab027bd Compare April 16, 2025 11:56

sschuberth requested changes Apr 16, 2025

View reviewed changes

sschuberth mentioned this pull request Apr 16, 2025

Various common utility refactorings #10181

Open

fviernau added 3 commits April 17, 2025 09:20

docs(node): Update function docs to no more refer to npm view

5d4d362

The use of `npm view` is an implementation detail of `getRemotePackageDetails()`. While at it, use a reference to `PackageJsonFile`. Signed-off-by: Frank Viernau <[email protected]>

chore(node): Add the missing PackageJson.optionalDependencies

d69d4f6

Signed-off-by: Frank Viernau <[email protected]>

refactor(nodoe): Split up parsePackage()

e76b872

Prepare for re-use. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the yarn-rewrite branch from ab027bd to 2139e12 Compare April 17, 2025 07:41

fviernau requested a review from sschuberth April 17, 2025 07:41

sschuberth enabled auto-merge (rebase) April 17, 2025 10:15

sschuberth approved these changes Apr 17, 2025

View reviewed changes

sschuberth merged commit cd56fa7 into main Apr 17, 2025
26 checks passed

sschuberth deleted the yarn-rewrite branch April 17, 2025 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(node): Re-write Yarn to no more rely on `node_modules` file hierarchy #9427

refactor(node): Re-write Yarn to no more rely on `node_modules` file hierarchy #9427

fviernau commented Nov 14, 2024 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

sschuberth Mar 21, 2025

fviernau Mar 24, 2025

sschuberth Mar 21, 2025

fviernau Mar 24, 2025

sschuberth Mar 24, 2025

fviernau Mar 24, 2025 •

edited

Loading

sschuberth Mar 24, 2025

fviernau Mar 24, 2025

sschuberth Apr 14, 2025

fviernau Apr 16, 2025

fviernau Apr 16, 2025

sschuberth Apr 16, 2025

fviernau Apr 17, 2025 •

edited

Loading

sschuberth Apr 17, 2025 •

edited

Loading

fviernau Apr 17, 2025


		override fun dependenciesFor(dependency: ModuleInfo): List<ModuleInfo> = dependency.dependencies.toList()
		override fun linkageFor(dependency: YarnListNode): PackageLinkage = PackageLinkage.DYNAMIC

refactor(node): Re-write Yarn to no more rely on node_modules file hierarchy #9427

refactor(node): Re-write Yarn to no more rely on node_modules file hierarchy #9427

Conversation

fviernau commented Nov 14, 2024 • edited Loading

codecov bot commented Mar 19, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

sschuberth Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor(node): Re-write Yarn to no more rely on `node_modules` file hierarchy #9427

refactor(node): Re-write Yarn to no more rely on `node_modules` file hierarchy #9427

fviernau commented Nov 14, 2024 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

fviernau Mar 24, 2025 •

edited

Loading

fviernau Apr 17, 2025 •

edited

Loading

sschuberth Apr 17, 2025 •

edited

Loading