Skip to content

fix(cli): use the resolved owner package for a component's plain-text import hint - #5294

Merged
cixzhang merged 3 commits into
facebook:mainfrom
andrskr:fix/cli-component-import-hint-ownership
Sep 13, 2026
Merged

fix(cli): use the resolved owner package for a component's plain-text import hint#5294
cixzhang merged 3 commits into
facebook:mainfrom
andrskr:fix/cli-component-import-hint-ownership

Conversation

@andrskr

@andrskr andrskr commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Problem

astryx component <Name>'s plain-text output always showed import {Name} from '@astryxdesign/core/...', even for a component owned by an integration package with no core equivalent. #4810 already fixed this for the JSON response (withOwnership in api/component/_adapter.mjs now resolves the specifier against the owning package's exports map), but the command's text formatter never used that value — it recomputed its own hint via resolveImportPath(coreDir, resolvedName), which only ever resolves against core.

Fix

component.detail's text-output branch in clients/cli/commands/component/index.mjs now reads result.data.import (the value withOwnership already resolved correctly) instead of recomputing it. One line.

Testing

Extended import-hint-correctness.test.mjs with a new describe covering integration-owned components end to end via runCli, using the same "root the fixture inside the package tree" pattern doctor.test.mjs already uses (Vite's server.fs.allow can't serve a real tmp-path config at runtime):

Confirmed all three fail without the one-line fix (reverted it locally and reran) and pass with it.

  • import-hint-correctness.test.mjs: 497/497 passed.
  • component-ownership.test.mjs, component-package.test.mjs, component-resolution.test.mjs: 44/44 passed.
  • eslint on both changed files: clean.

… import hint

`astryx component <Name>`'s text formatter recomputed the import hint via
the core-only resolveImportPath regardless of the component's actual
owner, so an integration-owned component always printed an
@astryxdesign/core import that did not exist. The JSON response already
resolves this correctly via withOwnership; the command now uses that
value instead of redoing the resolution.
@vercel

vercel Bot commented Aug 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
astryx Ready Ready Preview Sep 13, 2026 12:07pm UTC

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 21, 2026
@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Aug 21, 2026
cixzhang
cixzhang previously approved these changes Sep 3, 2026

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this fixes the misleading integration import hint by reusing the API’s resolved package. The focused regression and ownership suites pass.

[Reviewed by Robohands]

@github-actions github-actions Bot removed the needs:code-review High-risk change (new package/component/API) — needs human code review before merge label Sep 3, 2026
@cixzhang
cixzhang enabled auto-merge (squash) September 3, 2026 21:36
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

Current run: The current CI run concluded failure. Current analysis and preview links are unavailable.


Generated by PR Enrichment workflow | View current CI run

@cixzhang
cixzhang dismissed their stale review September 7, 2026 05:48

Stale automated approval from the previous reviewer; this PR needs a fresh current-head review.

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this fixes the misleading import hint by reusing the package Astryx already resolved across full, compact, and brief output. Focused regression and ownership checks pass.

[Reviewed by Robohands]

@cixzhang

cixzhang commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

The failed-jobs retry on this exact head reproduced the same repository-owned CLI type-surface error from its old base (api/search/search.mjs is missing the later matched/total fields). Your import-hint change is not the failing code, but this head cannot clear required CI as-is. Please rebase or merge current main into the branch without expanding the PR; the resulting head will need fresh required checks and exact-head review.

github-actions Bot added a commit that referenced this pull request Sep 9, 2026
@cixzhang
cixzhang merged commit 4265195 into facebook:main Sep 13, 2026
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants