fix(install): clear error when a component depends on an unpublished update-dependent#10427
Open
davidfirst wants to merge 8 commits into
Open
fix(install): clear error when a component depends on an unpublished update-dependent#10427davidfirst wants to merge 8 commits into
davidfirst wants to merge 8 commits into
Conversation
…update-dependent A visible workspace component can depend on a hidden lane update-dependent whose Ripple build failed (or never completed), so no package was published. bit install then failed with a cryptic pnpm 'No matching version found' for the unpublished 0.0.0-<hash>. Detect this up-front: for each checked-out component's snap dependency that is a lane update-dependent, not checked out, and whose local Version.buildStatus is not 'succeed', throw an actionable error suggesting 'bit import <id>' (which links it from source instead of fetching from the registry).
Code Review by Qodo
1. Hardcoded — in message
|
PR Summary by Qodofix(install): fail early on unpublished lane update-dependent dependencies WalkthroughsDescription• Detect hidden lane update-dependent snap deps that were never successfully built/published. • Fail bit install early with actionable error and bit import remediation. • Add e2e coverage for install failure on unpublished update-dependent cascade snaps. Diagramgraph TD
A["InstallMain._installModules"] --> B["Build manifests"] --> C["Check lane updateDependents"] --> D{"Unpublished update-dependent?"}
D -->|"yes"| E["Throw UpdateDependentBuildFailed"] --> F["User runs: bit import <id>"]
D -->|"no"| G["Run pnpm install"]
High-Level AssessmentThe following are alternative approaches to this PR: 1. Probe registry for package existence
2. Auto-import hidden update-dependents when needed
Recommendation: Current approach (local lane/updateDependents + local buildStatus gate with an explicit File ChangesEnhancement (1)
Bug fix (2)
Tests (1)
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…hared CLI formatter Address review feedback: - A locally-stale pending/failed buildStatus could wrongly fail install when the remote already built/published the snap. Refresh the suspect versions from the remote (reFetchUnBuiltVersion) before deciding they're unpublished. - Use errorSymbol/formatItem from @teambit/cli instead of hardcoding the Unicode symbol, per the CLI output style guide.
|
Code review by qodo was updated up to the latest commit 16b5328 |
…t guard BuildStatus.Skipped is treated as 'built enough' elsewhere (onlyIfBuilt in scope-components-importer, sources.get), so the install guard now treats both Succeed and Skipped as built and won't report a Skipped update-dependent as unpublished.
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Code review by qodo was updated up to the latest commit 7a54a97 |
…-manager failure Replace the pre-emptive per-install scan (which ran a dependency walk + remote refetch on every install) with a catch around the package-manager step: only when it fails with 'No matching version found' do we check whether the failing package is a component-dependency snap that isn't checked out, and if so surface the actionable 'bit import' message. Match against the package-manager error rather than lane.updateDependents, since the install's import phase may have already dropped the entry from the local lane object. This also removes the buildStatus/Skipped/refetch logic (the failure itself proves the package is unpublished), so there is zero overhead on successful installs. The e2e now uses the npm CI registry so the unpublished snap genuinely 404s.
|
Code review by qodo was updated up to the latest commit 8b4c264 |
…ager error bit import does not remove entries from lane.updateDependents (only snap/merge/reset and the explicit removeUpdateDependents command do, via addVersion). Fix the comment that wrongly attributed a missing entry to the import phase; the real reason to match the package-manager error is that it pinpoints the failing version and a consumer can keep pinning a never-published snap after the entry was promoted out of updateDependents.
|
Code review by qodo was updated up to the latest commit 704078f |
… error The matcher keys off the package-manager 'No matching version found' error, not lane.updateDependents membership, so it can legitimately fire for any snap dependency that isn't checked out and was never published (e.g. after a reset that rewound a component to a never-published snap). Reword the message to name that as the likely cause rather than asserting the component is an update-dependent. Rename the payload type FailedUpdateDependent -> UnpublishedSnapDependency accordingly.
|
Code review by qodo was updated up to the latest commit dff5520 |
Comment on lines
+542
to
+556
| private enrichUnpublishedSnapDepError(err: Error, components: Component[]): Error { | ||
| // checked-out components are linked from source, so they're never fetched from the registry. | ||
| const workspaceIds = this.workspace.listIds(); | ||
| const errMessage = err.message || ''; | ||
|
|
||
| const unpublished = new Map<string, { id: ComponentID; dependents: Set<string> }>(); | ||
| for (const component of components) { | ||
| for (const dep of this.dependencyResolver.getComponentDependencies(component)) { | ||
| const depId = dep.componentId; | ||
| if (!depId.version || !isSnap(depId.version)) continue; | ||
| if (workspaceIds.hasWithoutVersion(depId)) continue; | ||
| // only the dependency the package manager actually failed on: its package + snap version appear in the | ||
| // error (the manifest version is `0.0.0-<hash>`, so the raw hash is a substring). | ||
| if (!errMessage.includes(dep.packageName) || !errMessage.includes(depId.version)) continue; | ||
| const key = depId.toStringWithoutVersion(); |
There was a problem hiding this comment.
1. Misclassifies pnpm failures 🐞 Bug ≡ Correctness
enrichUnpublishedSnapDepError rewrites install failures based only on substring-matching (packageName + snap hash) and can therefore replace non-"no matching version" pnpm errors (e.g. network/auth/404) with UpdateDependentBuildFailed, masking the true root cause. This repo already treats non-ERR_PNPM_NO_MATCHING_VERSION codes (including FETCH_404) as potentially real outages/auth failures and avoids swallowing them, so this enrichment should be gated similarly.
Agent Prompt
### Issue description
`InstallMain.enrichUnpublishedSnapDepError()` rewrites package-manager failures into `UpdateDependentBuildFailed` using only `err.message.includes(dep.packageName)` + `err.message.includes(depId.version)`. This can incorrectly rewrite auth/network/registry failures (including pnpm 404/auth-ambiguous cases), hiding important remediation.
### Issue Context
- pnpm errors are wrapped by `pnpmErrorToBitError()` and the original pnpm error is attached on `cause`.
- Elsewhere in the repo, only `ERR_PNPM_NO_MATCHING_VERSION` is treated as the “unpublished snap” signal; other codes (even `FETCH_404`) are explicitly not treated as such because they may indicate auth failures/outages.
### Fix Focus Areas
- scopes/workspace/install/install.main.runtime.ts[448-570]
### Implementation notes
- Extract pnpm error code safely, e.g. `const code = (err as any).cause?.code ?? (err as any).code;`.
- Only attempt the unpublished-snap enrichment when `code === 'ERR_PNPM_NO_MATCHING_VERSION'` (and/or when the message includes an unmistakable marker like `No matching version found`).
- For all other codes, return the original error unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit b85689b |
Comment on lines
+29
to
+33
| return formatItem(`${id} (${shortVersion}) — required by: ${requiredBy}`, errorSymbol); | ||
| }) | ||
| .join('\n'); | ||
| const importCommand = `bit import ${unpublished.map(({ id }) => id).join(' ')}`; | ||
| return `unable to install the following component(s) — they are pinned to a snap that was never published to the registry, so there is no package to install: |
There was a problem hiding this comment.
1. Hardcoded — in message 📘 Rule violation ⚙ Maintainability
The new UpdateDependentBuildFailed error output hardcodes the Unicode em dash — in user-facing CLI text. This violates the requirement to avoid hardcoded Unicode symbols in CLI output and to rely on the shared formatting toolkit for consistent markers.
Agent Prompt
## Issue description
The CLI error message in `UpdateDependentBuildFailed.formatMessage()` hardcodes the Unicode em dash `—` in output strings.
## Issue Context
The compliance checklist requires using the shared CLI output formatting toolkit and avoiding hardcoded Unicode symbols for consistent styling and maintainability.
## Fix Focus Areas
- scopes/workspace/install/exceptions/update-dependent-build-failed.ts[29-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A visible workspace component can depend on a hidden lane
updateDependentsentry (created by "snap updates" / Ripple). Those entries aren't checked out, so a dependent must fetch them from the registry as a package — but a package only exists once Ripple built the entry successfully. When the build failed (or never completed),bit installdied with a cryptic pnpm error:Fix
After manifests are built and before pnpm runs, check each checked-out component's snap dependencies: if a dep is in
lane.updateDependents, isn't checked out, and its localVersion.buildStatusisn'tsucceed, throw an actionable error instead:Checked-out components are skipped (they're linked from source, not fetched), so the suggested
bit importremediation makes the next install succeed.Adds e2e scenario 21 to
update-dependents-cascade.e2e.ts.