Repository navigation
feat(cli): doctor integration components compares documented props with the code - #7676
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR can't be approved yet because the behavior it adds has no current spec on main: AST-039 is still a draft there and ends at FR11, and the FR12 this PR implements exists only in draft spec PR #7658. A maintainer needs to approve that requirement, or identify another current claim, settling whether doctor integration components should compare documented props with the TypeScript props as warnings with a data.propDrift field, and whether Core should enforce it through the known-drift ratchet test. Separately, please make the text output carry every propDrift field: docRequired and codeRequired appear in the JSON but never print, and with any finding the compared count and TypeScript version disappear (see propDriftBlocks in doctor.mjs), which AST-017 FR13 requires.
[Automated review]
| fields: [ | ||
| 'severity', | ||
| 'component', | ||
| 'prop', | ||
| 'kind', | ||
| 'message', | ||
| 'doc', | ||
| 'source', | ||
| 'docType', | ||
| 'codeType', | ||
| ], |
There was a problem hiding this comment.
docRequired and codeRequired are missing from this list, so a required_mismatch finding (and a not_in_doc finding whose requiredness is known) prints without them while --json carries them. AST-017 FR13 requires every field in the text projection, and the PR description says the text shows each field a finding carries. Listing both keys is enough, because the formatter prints false:
fields: ['severity', 'component', 'prop', 'kind', 'message', 'doc', 'source', 'docType', 'codeType', 'docRequired', 'codeRequired'],There was a problem hiding this comment.
Fixed at e7ce3a1. The findings show docRequired and codeRequired, and the compared count and TypeScript version print whether or not anything differs (in the summary beside findings, the [ok], or an [info] line when a failed check holds the [ok] back). clients/cli/commands/doctor-integration.test.mjs › "components prints every field of the props comparison that JSON carries" compares the JSON with the text field by field for each finding kind and each component not compared.
e881162 to
e7ce3a1
Compare
There was a problem hiding this comment.
This PR can't be approved yet because the behavior it adds still has no current spec on main: AST-039 is a draft there and ends at FR11, and the FR12 this PR implements exists only in draft spec PR #7658. A maintainer needs to approve that requirement, or identify another current claim, settling whether doctor integration components should compare documented props with the TypeScript props as warnings with a data.propDrift field, and whether Core should enforce it through the known-drift ratchet test. The earlier text-output finding is fixed: findings now print docRequired and codeRequired, and the compared count and TypeScript version print whenever the comparison runs. Nothing else in this review blocks approval.
[Automated review]
33b99ca to
6ac81a6
Compare
e7ce3a1 to
59d4101
Compare
There was a problem hiding this comment.
This PR can't be approved yet because the behavior it adds still has no current spec on main: AST-039 is a draft there and ends at FR11, and the FR12 this PR implements exists only in draft spec PR #7658. A maintainer needs to approve that requirement as current, or identify another current claim, settling whether doctor integration components and integrationComponentConflicts() should compare documented props with the TypeScript props as warning-only findings with an optional data.propDrift field and a [skip] line when no usable TypeScript is installed, and whether Core should enforce the same comparison through the known-drift ratchet test. The implementation itself checks out on this revision: no defects were found, exit codes are unchanged, and the text output carries every propDrift field.
[Automated review]
59d4101 to
cfdedf5
Compare
There was a problem hiding this comment.
This PR can't be approved yet because the behavior it adds still has no current spec on main: AST-039 is a draft there and ends at FR11, and both the FR12 this PR implements and the FR4 rule that a component doc's name is the export it documents exist only in draft spec PR #7658. A maintainer needs to approve those requirements as current, or identify another current claim, settling whether doctor integration components and integrationComponentConflicts() should compare documented prop tables with the TypeScript props as warning-only findings in an optional data.propDrift field, which export a doc's own table is compared with when its name differs from its file, and whether Core should enforce the comparison through the known-drift ratchet test. The implementation itself checks out on this revision: no defects were found, exit codes are unchanged, and the text output carries every propDrift field.
[Automated review]
c3aef0f to
1a7383c
Compare
cfdedf5 to
97474e5
Compare
There was a problem hiding this comment.
This PR can't be approved yet because the behavior it adds still has no current spec on main: AST-039 is a draft there and ends at FR11, and both the FR12 this PR implements and the FR4 rule that a component doc's name is the export it documents exist only in draft spec PR #7658. A maintainer needs to approve those requirements as current, or identify another current claim, settling whether doctor integration components and integrationComponentConflicts() should compare documented prop tables with the TypeScript props as warning-only findings in an optional data.propDrift field, which export a doc's own table is compared with when its name differs from its file, and whether Core should enforce the comparison through the known-drift ratchet test. The implementation itself checks out on this revision: no defects were found, exit codes are unchanged, the text output carries every propDrift field, and Core still yields exactly the 99 listed mismatches.
[Automated review]
1a7383c to
722fcd3
Compare
2143de3 to
515eab6
Compare
There was a problem hiding this comment.
This is ready to approve: doctor integration components compares every props table a package ships with the component's TypeScript props, as architecture:cli-surface INV33 requires, keeps exit codes unchanged, and never asks a doc for the props authors are told to skip. The rebase onto main holds: the ContextMenu entry that main fixed is gone from the known-drift list, Core yields exactly the 102 listed findings, and the CLI tests and checks pass. Nothing else is needed here; it can land after its base PRs, in the order the description gives, and should ship in the same release as integration check.
[Automated review]
c0b2461 to
2ed0495
Compare
515eab6 to
788903f
Compare
There was a problem hiding this comment.
This is ready to approve. doctor integration components now compares every props table a package ships with the component's TypeScript props, as architecture:cli-surface INV33 requires: it never asks a doc for the props authors are told to skip, says when it skipped the comparison, and keeps exit codes unchanged. The restack onto the base PR's new head leaves this change as it was, and the CLI tests, typechecks and repository checks pass on this revision, with Core yielding exactly the 102 listed findings. Nothing else is needed here: it can land after its base PRs in the order the description gives, and should ship in the same release as integration check.
[Automated review]
2ed0495 to
332eff7
Compare
788903f to
e0bf813
Compare
There was a problem hiding this comment.
This is ready to approve: doctor integration components now compares every props table a package ships with the component's TypeScript props, as architecture:cli-surface INV33 requires, never asks a doc for the props authors are told to skip, says when it skipped the comparison, and keeps exit codes unchanged. The restack changes only one test here, and the CLI tests, typechecks and published type check pass on this revision, with Core yielding exactly the 102 listed findings. Nothing else is needed here; it can land after its base PRs, in the order the description gives.
[Automated review]
332eff7 to
02eeac1
Compare
e0bf813 to
ec0c4fa
Compare
There was a problem hiding this comment.
This is ready to approve: doctor integration components now compares every props table a package ships with the component's TypeScript props, as architecture:cli-surface INV33 requires, never asks a doc for the props authors are told to skip, says when it skipped the comparison, and keeps exit codes unchanged. The restack leaves this change as it was, and its tests, the CLI test suite, the typechecks and the published type check pass on this revision, with Core yielding exactly the 102 listed findings. One optional follow-up: because the exempt-props test moved up to #7667, nothing here fails if key drops out of PROPS_NOT_DOCUMENTED, so declaring an own key?: string in one command-test fixture would guard it until #7667 lands. It can land after its base PRs, in the order the description gives.
[Automated review]
| ); | ||
| fs.writeFileSync( | ||
| path.join(tmpDir, 'components', 'AcmeBadge.tsx'), | ||
| "export function AcmeBadge(props: {label: string; tone?: 'neutral' | 'info'}) { return null; }\n", |
There was a problem hiding this comment.
Optional, until #7667 lands: since the restack moved the exempt-props test up to #7667, nothing at this revision fails if 'key' drops out of PROPS_NOT_DOCUMENTED, because Core declares no own key. Declaring one in this fixture, for example key?: string, would make this test guard INV33's key exemption: it should draw no finding.
There was a problem hiding this comment.
Done in bc10598: the fixture declares its own key?: string, and the test fails if key leaves PROPS_NOT_DOCUMENTED.
02eeac1 to
dbbf90d
Compare
There was a problem hiding this comment.
This looks ready to approve: doctor integration components now compares every props table a package ships with the component's TypeScript props, as architecture:cli-surface INV33 requires, warns about each difference without changing the exit code, and says when it skipped the comparison. The new key?: string fixture closes the last test gap, since the command test now fails if key leaves the exempt list. Nothing else is needed here; it can land after its base PRs in the order the description gives, and should ship no later than the release that adds integration check.
[Automated review]
…th the code `astryx doctor integration components` now runs the foundation prop comparison on each component doc, read with the project's own `typescript`, and warns about a documented prop the code lacks, a prop the package declares that the doc omits, and a wrong `required`. Components whose props cannot be read are listed with the reason, and the closing `[ok]` prints only when at least one prop table was compared. Without TypeScript it prints one line saying the comparison was skipped. Findings are warnings; JSON gains data.propDrift. Each finding carries both sides where they apply: the documented type of a prop the code lacks (`docType`), the code's type for a prop the doc omits as the source writes it, a union's members in the order the source declares them (`codeType`), and both flags of a wrong `required` (`docRequired`, `codeRequired`). The README's integration authoring table names the props check. Core's own component docs get the same comparison as a repository test, with the drift Core already has listed in core-prop-docs.known-drift.json; the list only shrinks.
…ps comparison The text view now shows each finding's docRequired and codeRequired, and says how many components were compared and with which TypeScript whether or not anything differs: in the summary beside findings, in the [ok] when nothing differs, and in an [info] line when a failed check holds the [ok] back. A test compares the JSON with the text for each finding kind and each component not compared.
`astryx docs cli/integrations/building-blocks/components/describe-the-component/check-the-props` shows how to run `doctor integration components`, what each finding means and how to fix it, and why a component can be listed as not compared. The component guides and the checks guide link to it, and the single-component guide adds the check to the commands to run after a change.
…props `doctor integration components` said "N documented prop(s) differ from the code", but N counts every finding, including a prop the doc leaves out. It now says "N prop(s) differ between the docs and the code". The guide's sample output is now a real run with one finding, so its summary agrees with the finding shown.
Two kinds of table were never compared, so a stale one passed under `[ok]`. A family doc whose `name` names another export of its file compared its own table, and its other entries only where discovery recorded a member: a `hiddenComponents` entry, or one whose source isn't built yet, was skipped. And a doc with `hidden: true` never reached the comparison, because discovery leaves it out. Now the doc's own record compares every entry no member record compares, and an entry whose source isn't found is listed as not compared with the reason. The command adds a record, under the doc's file name, for each doc discovery leaves out: a hidden one, or one whose source or own file doesn't load. The props guide says hidden docs and every family entry are compared, a prop a mapped type of the package's own builds is asked for, a prop the package's `defaultProps` fills counts as optional, and a deprecated prop is still asked for.
…ide a .doc.mjs The summary counted components by name, so two docs of one name (in two folders, or a hidden older one) read as one component: "3 prop(s) differ ... in 1 of 2". It counts each doc's component now. A `.doc.ts` beside a `.doc.mjs` of the same name was left out, though both ship and validate checks both; every doc discovery leaves out gets its own record now.
… source is missing
A family doc whose same-stem source is missing gets no record of its own,
but discovery still records the members its package exports, and those
records carry the doc's path. The doc then counted as compared, so its own
table and its `hiddenComponents` entries were neither compared nor listed.
Such a doc now has its own table and every entry no member record compares
compared, or listed as not compared ("no source file for <Name>").
…for a doc entry
A prop a mapped type builds from keys the comparison can't trace to the
package's source (a template literal key, say) draws no `not_in_doc`. The
command now lists such props under `[info] Props not checked for a doc
entry`, with the reason, and JSON adds `propDrift.propsNotChecked`
({component, doc, reason, props}); the `[ok]` line then says it leaves
them out. The props guide says which mapped props are asked for: not one
whose keys another package defines, behind an alias or not. Four of the
comparison's tests move here, from the base, to keep it under 2,000 lines.
…ry finds a member's A family entry discovery gives no record of its own, such as a `hiddenComponents` entry, was read only from `<Entry>.tsx`, the family's own file or the folder's index. When its export is declared in another file of the folder (`components/menu-parts.tsx`, re-exported from the package's entry), it was listed as not compared, while the same entry, not hidden, was compared from the source discovery finds. It now falls back to the `.tsx` in the doc's folder that declares it, as discovery does for a member.
… optional The comparison under this reads a prop that only some members of a union require as optional, since a caller can leave it out, and holds every prop a doc entry names to that entry's `required`. The props guide's `required_mismatch` row says both: name the variants that need such a prop in its description, and give props separate entries when their flags differ. The typed-`any` test adds `forwardRef` components whose props are `Omit` or `Partial` of `any`, which the base now lists, and a polymorphic component whose props React's `ComponentPropsWithoutRef<E>` names, which stays listed. Three of the comparison's tests move here, from the base, to keep it under 2,000 lines (the `defaultProps` one with the first commit).
The repository test over Core's own docs read only docs beside a same-stem `.tsx`, so a doc whose `name` names an export in another file (`Resizable.doc.mjs`) was never compared, and a new mismatch there would pass. It now reads every `.doc.mjs` under Core's `src`, as the command reads a package's docs: Resizable's two existing mismatches join the known list, and the four docs whose own export has no same-stem source are pinned as listed, not compared.
The test of the exempt props moved up to the prop types PR, so nothing here failed if `key` left the list of props a doc never needs. A fixture component in the command test now declares its own `key`, and the test's exact findings would gain `key not_in_doc` without it.
User need
#7663 adds a check that compares each component doc's props with the component's TypeScript props, on #7681, which reads the package's TypeScript project. Nothing runs it yet, so an integration still learns about a stale prop table only when someone reads the code.
Problem and solution fit
astryx doctor integration componentsnow runs #7663's comparison on every component doc. It first finds each doc's prop tables:propsdocument the export itsnamenames, even when another file beside it holds that export;use…) documents parameters and returns, not props, and isn't a table.It prints each finding as a warning that names the component, the prop, the doc and source files, and the documented type. It lists the components it can't read, each with its reason. It prints the closing
[ok]only when at least one prop table was compared, never beside a finding or after a failed check. Without TypeScript, it prints one line saying the comparison was skipped and why. It never reports a pass it didn't check.Core gets the same comparison as a repository test,
packages/cli/test/core-prop-docs.test.mjs. The 102 mismatches Core has today, over 182 prop tables, are listed incore-prop-docs.known-drift.json. The list only shrinks: a new mismatch fails the test, and so does a fixed one until its line is removed.Current specification
Implements
architecture:cli-surfaceINV33, "Documented component props agree with the source" (AST-039 FR12, which now points to it; DEC-5), with #7663, #7684 and #7667. Its evidence row names the prop-drift, type-text, doctor-integration and core-prop-docs tests, which this PR's tests satisfy.Public delta and caller burden
astryx doctor integration componentsprints new warnings, then a line that says how many components were compared and with which TypeScript: in the summary beside the findings, as[ok] Documented prop names and required flags match the code in N component(s) (TypeScript x.y).when nothing differs, or as[info] Props compared in N component(s) (TypeScript x.y).when a failed check holds the[ok]back. Without a usable TypeScript it prints[skip] Props not compared: …, naming the installed version when that TypeScript has no JavaScript compiler API (TypeScript 7). Exit codes are unchanged, because findings are warnings.data.propDrift={status: 'checked' | 'skipped', reason?, typescript?, components, findings[], unchecked[]}. It is present when the manifest loads and declares a components root.{component, prop, kind, severity: 'warning', message, doc, source, docType?, codeType?, docRequired?, codeRequired?}, and the text shows every field it carries.typescriptit already has.Example: an Acme package's
AcmeButtondeclareslabel: string. Its doc leaveslabeloptional and lists anelevationprop the code doesn't have:The command still exits 0.
Design and alternatives
api/integration/_adapter.mjs(cli-surface INV21).authoring-checks.mjsonly attaches the report.astryx integration check --strict(feat(cli): integration check runs every source check for a package's build, with --strict #7665).Evidence
doctor-integration.test.mjs: text, JSON, the skip line, no[ok]when nothing was compared and none beside findings, and a test that compares the JSON with the text for each finding kind and each component not compared, field by field (test-utils/text-projection.mjs).prop-drift.test.mjs(4 tests ofcomparePropDocs): a family doc's entries and hooks, member records, a doc whosenamenames an export in another file, and a table for one component in each of two docs.authoring-checks.test.mjs, and the Core ratchet test above.not_in_doc, 26required_mismatchand 3not_in_code.Since the first automated review
[ok]prints only when at least one prop table was compared.propDriftis present when the manifest loads and declares a components root.[ok] Documented propline prints beside a finding.propDrift(AST-017 FR13): each finding'sdocRequiredandcodeRequired, and how many components were compared and with which TypeScript, whether or not anything differs. A test compares the JSON with the text for each finding kind.A member with no source file
TypeError. It is now listed as not compared ("no source file for "), and the rest of the doc is still compared. A test covers it and failed before. Discovery doesn't produce such records on main yet; a family-member change in review will.Props a mapped type builds
ownis decided. The props guide'snot_in_docrow here says so.Authoring docs
astryx docs cli/integrations/building-blocks/components/describe-the-component/check-the-props, shows how to run the props check and read its output. It says what each finding means and how to fix it, and why a component can be listed as not compared, with the fix for each reason. Its examples are real output.doctor integration componentsto the commands to run after a change, and the checks guide points to it for fixing a props finding.Every props table
[ok]. A family doc whosenamenames another export of its file compared only its own table, and its other entries only where discovery recorded a member: ahiddenComponentsentry, or one whose source isn't built, was skipped. And a doc withhidden: truenever reached the comparison, because discovery leaves it out.[ok]. Both failed before.defaultPropsfills counts as optional (feat(cli): compare documented component props with the TypeScript source #7663, which reads it throughmemoandlazytoo), and a deprecated prop is still asked for. Core and Lab are unchanged..doc.tsbeside a.doc.mjsof the same name was left out, though both ship and validate checks both. It gets its own record now.After #7663's review of mapped props
docType,codeType,docRequiredandcodeRequired. The printing gives the code's type as the source writes it, a union's members in source order, a method prop's function type rather than its return type, and an intersection's narrowed type. They are in this PR's first commit, the one that adds them to the JSON and the text. Core and Lab output is byte-identical.invalid_componenterror). It still records the members its package's entry exports, and those records carry the doc's path, so the doc counted as compared. Its own table and itshiddenComponentsentries were then neither compared nor listed.comparePropDocsnow compares the own table and every entry no member record compares for such a doc, or lists them as not compared ("no source file for "). A command test with that package shape failed before.After #7663's review of mapped-prop ownership
propsNotChecked. This PR prints them under[info] Props not checked for a doc entry, with the reason and the props, and the[ok]line then says "apart from the props not checked". The JSON docs, the README row and the props guide say so. A command test covers it and failed before.After the review of hidden entries in sibling files
hiddenComponentsentry, was read only from<Entry>.tsx, the family's own file or the folder's index. When its export is declared in another file of the folder (saycomponents/menu-parts.tsx, re-exported from the package's entry), it was listed as not compared, while the same entry, not hidden, was compared from the source discovery finds. It now falls back to the.tsxin the doc's folder that declares it, the same lookup discovery uses for a member. A test with a hidden entry in a sibling file failed before. Core and Lab output is byte-identical.After the review of the Core test
[info] Props not fully checked in N component(s), counting each component once, and the[ok]line says it leaves those props out. feat(cli): compare documented component props with the TypeScript source #7663 now lists a prop only some variants require there, and the props guide'srequired_mismatchrow says so, asking authors to name the variants that need it in its description. The command test covers both reasons..tsx. It now reads all 238, as the command reads a package's docs, so six more tables are compared. Resizable's two existing mismatches join the known list, and the four docs whose own export has no same-stem source are pinned as listed, not compared. A new mismatch anywhere in Core now fails the test. It failed before without the two known entries.anytest addsforwardRefcomponents whose props areOmitorPartialofany, which feat(cli): compare documented component props with the TypeScript source #7663 now lists; they failed before.After the round-21 review
ContextMenu.itemsandDropdownMenu.items, documented as required, are flagged and join the known list. The props guide'srequired_mismatchrow says such a prop counts as optional and its description should say which variants need it. It also says each prop of an entry that names several is listed, and asks authors to give each prop its own entry. The JSON docs say the same, and the command test's second not-checked reason is now a two-name entry.doctor integration componentsalso checks each props table against the TypeScript props.defaultPropstest moved here from feat(cli): compare documented component props with the TypeScript source #7663 and sits where it was, with this PR's check of the finding's fields.After the round-22 review
[info] Props not checked for a doc entry in N component(s), and the JSON docs and the command test match. The props guide'srequired_mismatchrow says an entry that names several props makes the same claim about each, and to give props separate entries when their flags differ.After main moved
required: truefrom ContextMenu'sitems, which is required in only one of its modes. This PR's Core test then reported that known entry as fixed, as it is meant to. The entry is deleted fromcore-prop-docs.known-drift.json, andDropdownMenu.itemsstays. The chain is rebased onto main 9b8b723. The skip test's two imports and its line in the test file's header now come with that test here, since feat(cli): compare documented component props with the TypeScript source #7663 dropped them. Nothing else in this PR changed.Restacked
Restacked
ComponentPropsWithoutRef<E>names, which stays listed as taking its names from a type parameter.Restacked
After the latest review
keyleft the list of props a doc never needs. A command-test fixture now declares its ownkey, so the test's exact findings would gainkey not_in_docwithout the exemption; it fails with the exemption removed.Merge order
The order is required:
doctor integration components, stacked on feat(cli): compare documented component props with the TypeScript source #7663), then feat(cli): read two written TypeScript types as one when they differ only in how they are written #7684 (reading two written types as one, stacked on feat(cli): doctor integration components compares documented props with the code #7676), then feat(cli): doctor integration components judges documented prop types against the code #7667 (prop types, stacked on feat(cli): read two written TypeScript types as one when they differ only in how they are written #7684).validate, stacked on feat(cli): check each authored doc against its published authoring type #7664).integration check) last, after a rebase that adds these PRs' fields and codes to its failures list. Merged earlier, it breaks main.This PR must ship in the same release as
integration check. If it slips past that release, it can ship only in a minor, because its findings failintegration check --strict.Overlap with open PRs
test-utils/text-projection.mjs, and the line that imports it indoctor-integration.test.mjs, are byte-identical in feat(cli): doctor integration validate checks each doc against its published authoring type #7673, so the two merge cleanly in either order.api/integration/authoring-checks.mjs, feat(cli): install once, run anywhere — every copy hands a project's run to the release its Core matches #7649 renamesfindCoreDirtofindProjectCoreDiron the line after the new block. Inassets/docs/tree/checks.doc.mjs, it drops thenpxprefix in the row this PR extends.astryx componentprints #7623, fix(cli): init writes agent docs every agent reads, at the project root, never in the home folder #7497, spec(AST-039, cli-surface): integration docs are found by their documented names and checked in one command #7658, feat(cli): a component doc can name a differently named export of the file beside it #7659, fix(cli): integration verify reads exports with the reader discovery uses #7660, feat(docsite): put the integration guide's pages under CLI Integrations in the sidebar #7661, feat(cli): integration family members are components under their own names #7674, or the other PRs in this set.Size
1,978 changed lines. 104 of them are
packages/cli/test/core-prop-docs.known-drift.json, a data file the Core test reads.Scope
Testing
The CI test jobs run only for pull requests into main, so this PR's tests run in CI once it is retargeted after #7663 merges. On main 9b8b723 with #7681, #7663 and this branch: copyright, sync-exports, the published
./apitype check,readme:check, the four CLI typechecks (strict, json-api, authoring-contract, template-docs),check:changesets, the lint job's changeset coverage check (run on the merge commit GitHub builds),check:cli-structure,check:repo, eslint on the changed files, prettier on the new files, and the fullpnpm vitest run --project nodesuite: 10,321 tests pass. The 21 failures are in the npm pack, packed real-world package, theme resolution, CSS graph and browser accessibility tests, which fail the same way on main 9b8b723 on the same machine (they need a package registry and a browser it doesn't reach). Earlier in review, with all nine PRs merged into main, the fourdoctor integrationsubcommands were run in text and JSON on 64 package shapes, against main: templates output is byte-identical everywhere;validateandcomponentsonly add their new section and JSON field;docsdiffers only where the doc-links PR fixes a link warning.integration checkexits 1 exactly when one of the four would, each of its sections is byte-identical to that subcommand's output, and onlyintegration check's three list tests fail until its planned rebase.