fix(core,cli): four defects from the Next.js 16 adoption report - #4409
Draft
AKnassa wants to merge 4 commits into
Draft
fix(core,cli): four defects from the Next.js 16 adoption report#4409AKnassa wants to merge 4 commits into
AKnassa wants to merge 4 commits into
Conversation
`astryx docs --list` was documented usage but commander rejected it with `unknown option '--list'`, exiting 1: the `docs [topic] [section]` command was registered with no `.option()` calls at all, even though a `docs.list` branch already existed and the bare `docs` invocation reached it. Wire the flag to that branch, so `--list` also wins over a positional topic. Refs facebook#4276
`--out` was described as "Output CSS file path", but every emitted artifact — the `.js`, the `.d.ts` and the `.variants.d.ts` — is written to that path's dirname, so the flag is closer to an output directory named by the CSS file. Use the wording the reporter asked for. Refs facebook#4276
`@astryxdesign/core` declares `@stylexjs/stylex` as a required peer with no
`peerDependenciesMeta`, but every documented install line omitted it, so
following the quick start under pnpm left a broken install. Name it on each
package-manager variant, and add a drift guard that stands itself down if
StyleX ever moves to `dependencies` — that question is separately contested
and is deliberately untouched here.
`astryx doctor` already flagged the missing peer generically, but could not
say what to install: `missing.map(m => m.split('@')[0])` splits a scoped name
on its leading `@`, so the remediation rendered as `npm install ` with no
package name at all, for every scoped peer. Fixed at the source rather than
adding a second, redundant stylex-specific check.
Refs facebook#4276
`BannerStatus` is extensible through `BannerStatusMap` module augmentation,
but both lookup tables were fixed `Record`s over the four built-in statuses.
A consumer-added status therefore missed on both: the `role` went `undefined`,
dropping the banner's live-region semantics entirely, and — worse — the icon
went `undefined` too, so `<Icon icon={undefined}>` threw and the banner did
not render at all. The role bug was unreachable behind the crash.
Both maps become `Partial` with named fallbacks at the read site, so an
augmented status announces as `role="status"` with the neutral icon. The
trade-off is that the four built-ins lose compile-time exhaustiveness; each
one keeps a role test to cover that.
Refs facebook#4276
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
community
meta-cla
Bot
added
the
CLA Signed
Contributor
PR Analysis ReportModified ComponentsBanner
Bundle Size Summary
Accessibility AuditStatus: 1 accessibility violation(s) found — 1 serious. Banner - 1 issue(s)
Generated by PR Enrichment workflow | View full report |
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.
Four independent defects from the adoption report, as four separate commits so they can be split or dropped individually.
1.
astryx docs --listcrashedIt was documented usage, but the command was registered with no options at all, so commander rejected the flag and exited 1 — even though a list branch already existed and the bare
docsinvocation reached it. Now wired up, and--listwins over a positional topic.2.
theme build --outhelp text was misleadingIt said "Output CSS file path", but all four emitted artifacts follow that path's dirname. Reworded to the phrasing the reporter asked for.
3. The documented install line omitted a required peer
@astryxdesign/coredeclares@stylexjs/stylexas a required peer with nopeerDependenciesMeta, but every documented install line left it out — so following the quick start under pnpm gives you a broken install. Added to each package-manager variant, with a drift guard that stands itself down if StyleX ever moves todependencies, so it doesn't prejudge the separate open question there.And a real bug found underneath it. The brief for this said "doctor has no stylex check". That's literally true of a grep, but wrong about behaviour:
checkPeerDepsalready flags it generically. What it couldn't do is say what to install —missing.map(m => m.split('@')[0])splits a scoped name on its leading@, so the first segment is empty and the remediation rendered asnpm installwith no package name, for every scoped peer. Fixed at the source rather than adding a second, redundant check.Deliberately not done: escalating that check from
warntofail. Missing StyleX is a hard runtime failure under pnpm so there's an argument for it, but it changesastryx doctor's CI exit code and nobody decided that.4. Banner silently dropped ARIA role on an augmented status
BannerStatusis extensible by module augmentation, but the lookup tables were fixed records over the four built-ins.This is worse than the report describes. The role going
undefinedis real — but the icon map has the identical defect, and<Icon icon={undefined}>throws, so an augmented-status Banner didn't render at all. The role bug was unreachable behind the crash. Both maps now fall back, so an augmented status announces asrole="status"with the neutral icon.Trade-off: the four built-ins lose compile-time exhaustiveness. Each keeps a role test to cover that.
What's deliberately left out
The report has ten findings plus seven follow-ups. Three are already dead — finding 4 was never broken (it uses
console.error, with a test), and 8 and 9 were fixed by merged #4279 and #4280 within a day of filing. The rest (the build timestamp, accent inversion in dark mode, a--color-focus-ringtoken, a success Button variant, and every follow-up item) each need a maintainer decision, and several already have one leaning against.How to check it
pnpm vitest runon the touched areas: docs 9, build-theme 19, install-line guard 5, doctor 27, Banner 36 + 48 consumers — all green. Full repo suite run alone: 382 files / 7755 tests, exit 0.The zh Banner description is my own translation and hasn't been reviewed.
Refs #4276