Conversation
Fix the out-of-memory errors with the why command
There was a problem hiding this comment.
Pull request overview
Updates the Bit workspace dependency policy to newer @pnpm/* package versions, aiming to address out-of-memory issues encountered when running pnpm’s why-style dependency analysis within this repo’s pnpm/Bit integration.
Changes:
- Bumped multiple
@pnpm/*dependencies (client/config/core, lockfile utilities, store/worker, reporting/listing) to newer patch/minor versions. - Updated pnpm-related graph/list/hierarchy packages that are likely involved in
why/dependency tree rendering.
The pnpm API changed: buildDependenciesHierarchy was replaced by buildDependentsTree which requires importerInfoMap and lockfile params and returns DependentsTree[] instead of a record. Updated imports and rendering to use renderDependentsTree accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| nameFormatter ({ manifest }) { | ||
| return manifest?.componentId | ||
| ? `${manifest.componentId.scope}/${manifest.componentId.name}` | ||
| : manifest.name |
There was a problem hiding this comment.
The nameFormatter function accesses manifest.name as a fallback when manifest.componentId is not available. However, there's no null check for manifest itself before accessing manifest.name. If manifest is undefined or null, this will throw a runtime error. Consider adding a null check: manifest?.name ?? 'unknown'.
| : manifest.name | |
| : (manifest?.name ?? 'unknown'); |
| const projectPaths = importerIds.map((id) => join(opts.lockfileDir, id)); | ||
| const importerInfoMap = new Map<string, ImporterInfo>(); | ||
| for (const importerId of importerIds) { | ||
| const pkgJson = tryReadPackageJson(join(opts.lockfileDir, importerId)); |
There was a problem hiding this comment.
The tryReadPackageJson function expects a package directory path, but it's being passed the importer path directly. For importers, the package.json should be read from the importer directory itself, which is join(opts.lockfileDir, importerId). However, this assumes the importer directory contains a package.json at its root. Verify that this path resolution correctly locates the package.json file for all importer types.
| "mime": "2.5.2", | ||
| "mini-css-extract-plugin": "2.2.2", |
There was a problem hiding this comment.
The dependency entries appear to have been reordered alphabetically. While this change doesn't affect functionality, it creates unnecessary diff noise. Consider maintaining the original ordering of dependencies unless there's a specific reason to sort them.
| "mime": "2.5.2", | |
| "mini-css-extract-plugin": "2.2.2", | |
| "mini-css-extract-plugin": "2.2.2", | |
| "mime": "2.5.2", |
| nameFormatter ({ manifest }) { | ||
| if ('componentId' in manifest) { | ||
| const { scope, name } = manifest.componentId as { scope: string; name: string }; | ||
| return `${scope}/${name}`; | ||
| } | ||
| return manifest.name; |
There was a problem hiding this comment.
nameFormatter uses 'componentId' in manifest and then destructures manifest.componentId. The in operator is true even when the property exists but is undefined/null, which would cause a runtime exception during destructuring. Consider guarding on the value/shape of manifest.componentId (and falling back to manifest.name / a safe default) before destructuring.
| nameFormatter ({ manifest }) { | |
| if ('componentId' in manifest) { | |
| const { scope, name } = manifest.componentId as { scope: string; name: string }; | |
| return `${scope}/${name}`; | |
| } | |
| return manifest.name; | |
| nameFormatter({ manifest }) { | |
| const componentId = (manifest as any).componentId; | |
| if ( | |
| componentId && | |
| typeof componentId === 'object' && | |
| typeof componentId.scope === 'string' && | |
| typeof componentId.name === 'string' | |
| ) { | |
| return `${componentId.scope}/${componentId.name}`; | |
| } | |
| return manifest.name ?? 'unknown'; |
| }); | ||
| it('should return paths to subdependency', () => { | ||
| expect(helper.command.dependenciesUsage('is-number')).to.contain('is-number 6.0.0'); | ||
| expect(helper.command.dependenciesUsage('is-number')).to.contain('is-number@6.0.0'); |
There was a problem hiding this comment.
The bit deps usage e2e assertion only checks that the output contains is-number@6.0.0, but this PR changes the output format to a reverse dependents tree. To prevent regressions, the test should also assert at least one structural aspect of the new output (e.g., that the queried package is rendered as the root and that a known dependent appears under it).
|
I had to downgrade all the other pnpm dependencies as one of the tests started failing. I am not sure what change in pnpm causes that issue. Here are results of the investigation done by claude for later: https://gist.github.com/zkochan/3b4114ebad28452198dcd2f186dec4ea |
bit whynow shows a reverse dependency tree. The searched package appears at the root with its dependents as branches, walking back to workspace components. This replaces the previous forward-tree output which was noisy and hard to read for deeply nested dependencies.Also with the complete rewrite of the dependency tree builder we have fixed the out-of-memory errors that were frequent with the why command. The command also became a lot faster.
Proposed Changes
Related PRs:
pnpm whytree and improvelist/whyoutput pnpm/pnpm#10615--depthoption topnpm whyto limit display depth pnpm/pnpm#10627