chore: extract TraversalNodeModuleCollector for fallback project parsing
#9440
Conversation
… `env: { COREPACK_ENABLE_STRICT: "0", ...process.env },` to allow `npm list` to work across environments. extract fallback node collector (Traversal) to separate class due to differing parsing logic from NPM collector
🦋 Changeset detectedLatest commit: 2e8ceea The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ep-prodpath # Conflicts: # packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
… with PM (new) or "auto" (current logic)
packages/app-builder-lib/src/node-module-collector/yarnNodeModulesCollector.ts
Show resolved
Hide resolved
| import { TraversalNodeModulesCollector } from "./traversalNodeModulesCollector" | ||
|
|
||
| export class BunNodeModulesCollector extends NpmNodeModulesCollector { | ||
| export class BunNodeModulesCollector extends TraversalNodeModulesCollector { |
There was a problem hiding this comment.
@beyondkmp note: this Bun collector uses the Traversal collector as base class. It returned far more node_modules in the collection that what npm list returned AND the original explicit PR implementation for Bun by a community member was manual traversal as well. Decided to leave it as-is. Albeit I just noticed that the logging below needs to be updated.
There was a problem hiding this comment.
Interesting, why is it so much more? It seems there's an issue with npm list under Bun.
There was a problem hiding this comment.
Honestly, I couldn't figure that out but I do intend to investigate it further.
Within the scope of this PR, using TraversalNodeModulesCollector made sure none of the test snapshots changed.
I'll create a follow-up PR for COREPACK_STRICT_ENABLE=0 and we can retest both Yarn v1 and Bun approaches in that PR
# Conflicts: # packages/app-builder-lib/src/util/appFileCopier.ts
# Conflicts: # packages/app-builder-lib/src/node-module-collector/bunNodeModulesCollector.ts # packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts # packages/app-builder-lib/src/util/appFileCopier.ts # test/src/helpers/packTester.ts
| } | ||
|
|
||
| // 2) upward hoisted search, then 3) downward non-hoisted search | ||
| return (await this.upwardSearch(parentDir, pkgName, requiredRange)) || (await this.downwardSearch(parentDir, pkgName, requiredRange)) || null |
There was a problem hiding this comment.
Is this only reachable during manual traversal? Because if pnpm isn't hoisted, reaching this point will definitely cause issues.
There was a problem hiding this comment.
Yes, it's used in pnpm collector's getProductionDependencies. What's the issue here when pnpm isn't hoisted? We handle that usecase with the subsequent downward search.
NpmDependency