facebook / facebook/astryx

[Infra] Only core is typechecked and fully linted — Lab crosses into a stricter world at promotion time

Open
#5,156 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

discussion needs-scoping
Dominant language
TypeScript
Stars
13.2k
Forks
1.1k
Avg merge
1d 14h
Merged PRs (30d)
669

Description

The gap

CI typechecks and fully lints only packages/core/src. Every other package —
lab above all — is built and tested, but is never typechecked as a whole and never
sees the @astryx/* or @eslint-react/* rule blocks. Code can therefore sit on main
for months in a state that would not pass review if the same lines lived in core.

This surfaced while promoting Bottom Sheet from Lab to Core (#5080). The promotion moved
files that had passed CI for weeks; the moment they landed under packages/core/src, CI
failed and the ruleset demanded a batch of changes that had nothing to do with the
promotion itself.

I'd like direction on whether we want to close this gap and how far, rather than a fix —
see "What I'm asking for" at the bottom.

Typecheck

.github/workflows/ci.yml runs typecheck:docs for core/lab/charts, plus
@astryxdesign/core typecheck — the step is literally named "Typecheck core (including
tests)"
. @astryxdesign/lab typecheck exists as a script and is never invoked. Same for
charts, richtext, vega.

The consequence, concretely: #5130 (feat(lab): size the Bottom Sheet scroll area to the visible sheet) landed a test helper with a cast TypeScript rejects:

function resizeEntry(...): ResizeObserverEntry {
  return {
    borderBoxSize: [{blockSize: borderBoxHeight, inlineSize: 100}],
    contentRect: rect({top: 0, bottom: contentBoxHeight}),
  } as ResizeObserverEntry;  // TS2352 — borderBoxSize is readonly, neither type overlaps
}

Green in lab, instant CI failure in core. Nobody wrote a bug; the file just crossed a
boundary where the checks differ.

Lint

The @astryx/* plugin block, the @eslint-react/* block and the type-aware
@typescript-eslint/* block in eslint.config.js are all scoped to
packages/core/src/**. packages/lab/src matches only the base config and the copyright
rule.

Promoting Bottom Sheet needed suppressions or rewrites for five rules the code had never
been checked against: @astryx/no-classname-clobber, @eslint-react/set-state-in-effect,
@eslint-react/exhaustive-deps, @eslint-react/no-use-context,
@eslint-react/no-context-provider.

How big is it, actually

Smaller than it looks, which is why I think it's worth discussing now.

Typecheck — with pnpm -F @astryxdesign/core build run first:

package typecheck errors on main
lab 5, in 2 files
charts 0
richtext 0
vega 0

Of lab's five: 4 × TS7016 in src/__tests__/labApiContractDrift.test.tsx (untyped
.doc.mjs imports — one shared .d.ts fixes all four) and 1 × TS2352, which is exactly
the failure above. Enabling this check today would have caught #5080's CI break at the
moment #5130 was authored.

⚠️ Ordering matters: without core built, lab reports 274 errors, 165 of them TS2307
"cannot find module". Any CI step must run after the build, or the number is meaningless.

Lint — running core's rule blocks against packages/lab/src (strict mode):
267 errors + 17 warnings, top rules:

53  @astryx/no-hardcoded-i18n-string        15  @astryx/no-nullish-jsx-guard
47  @typescript-eslint/no-unnecessary-type-assertion   8  @eslint-react/no-use-context
38  @typescript-eslint/array-type            6  @eslint-react/naming-convention-ref-name
22  @astryx/boolean-prop-naming              6  @eslint-react/no-missing-context-display-name
16  @eslint-react/set-state-in-effect        5  @eslint-react/no-unstable-default-props
15  @eslint-react/no-array-index-key         4  @eslint-react/no-context-provider

So typecheck is nearly free; lint is a real backlog and the interesting decision.

Suggested shape (to react to, not to implement)

Add the missing packages to CI, but report-only at PR time — the step runs, annotates,
and does not block the merge. That gets the signal in front of authors immediately without
holding PRs hostage to 267 pre-existing violations, and lets us tighten to blocking
per-rule or per-package once the backlog drains.

For lint specifically, eslint.config.js already has an isStrictMode switch
(ASTRYX_STRICT_LINT=1 / CI=true) that flips severities — a "warn in lab, error in core"
tier may fall out of the existing machinery rather than needing new machinery.

Open questions

  1. Is Lab deliberately a lower bar? A genuine answer of "yes, Lab trades rigor for
    speed and paying at promotion time is the intended design" closes this issue, and that's
    a fine outcome. But then promotion cost should be explicit in the promotion checklist,
    because right now it's a surprise discovered in CI.
  2. Non-blocking forever, or a ratchet? Report-only checks that nobody reads are worse
    than no checks. If we want a ratchet, someone has to own the burn-down.
  3. Which rules are genuinely core-only? Some of the 18 look universal
    (no-classname-clobber, set-state-in-effect). Others may be deliberate core policy
    (no-hardcoded-i18n-string — 53 hits — is plausibly "stable packages are localized,
    Lab isn't yet"). This wants a rule-by-rule call from someone with the history, not a
    blanket copy of the core block.
  4. Baseline or fix-forward? A checked-in baseline file unblocks enabling everything
    today; fix-forward is cleaner but keeps the check off until the backlog clears.
  5. Scope beyond lab? charts/richtext/vega are clean right now — adding them costs
    nothing today and stops the same drift there.

What I'm asking for

This is a gap report, not a work item. I'm looking for direction and discussion —
please don't take it as a request to send a PR. The five-line typecheck fix is trivially
doable, but doing it without answering Q1 and Q3 just relocates the decision into a diff.
Once there's a call, I'm happy to do the work.


Found while rebasing #5080 (feat(core): promote Bottom Sheet from Lab); the CI failure
there is fixed and unblocked, so nothing is waiting on this.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read .github/workflows/ci.yml and eslint.config.js first, then inspect the typecheck scripts for lab, charts, richtext, and vega. Build core before running the lab typecheck, and reproduce the reported lint counts for packages/lab/src. Done means the open questions have a maintainer decision and a bounded implementation scope; this issue explicitly is not yet a work item.

Written by the indexing model from the issue text.

Assessment

Tech stack
eslint, github-actions, typescript
Domain
ci-cd, tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.