Fix #155: record-props is THE output form — nameable props type for every component - #156
Merged
Conversation
Benchmark: ✅ PASS
|
commit: |
…meable props type
Whether a component binds as `type props = {...}` + `external make:
React.component<props>` or as labeled args was decided solely by "extends
*HTMLAttributes / shares a base" (recordProps gate in emit.mjs). A plain-props
component (blend's AccordionItem) never got a nameable `props` type, blocking the
wrapper/override pattern (type props = Lib.Item.props + <Item {...props} onClick=mine/>).
Every component with >=1 prop now emits the record form — NO FLAG, one output form
only (per review discussion: a second output mode doubles the test surface and forces
every consumer regen script to carry an option, for zero benefit — the two forms are
behaviorally identical for JSX, v4 lowers to make/props either way).
- emit.mjs: the recordProps gate is unconditional (`|| ir.props.length > 0`). A
props-less component keeps labeled args (a record can't be empty; extraction skips
those as `no-props` anyway). A generic prop parameterizes the record (type props<'a>).
- cli.mjs: `--record-props` is ACCEPTED as a no-op so any script that adopted it
during the pkg.pr.new preview keeps working; no new flag surface.
- All 105 goldens regenerated (form-only flip) and ALL COMPILE on ReScript — the
compile run is the safety net for record-form edges (reserved words keep
@as("type") type_, tyvars declared via type props<'a>).
- Benchmark: all 9 packages compile, usable/review/broken metrics IDENTICAL, flag
markers byte-equal (910=910 on blend) — pure form churn, accepted via bench:update.
- #65 fallback note: a generic/imperfect variant-branch field now falls back to the
record form, where the tyvar is DECLARED (type props<'b>) — still compile-safe.
Verified: the motivating wrapper pattern compiles against the generated output
({...props, subtext: ?mine} spread-with-override in the sandbox). Fixture:
record-props (renamed from record-props-flag); smoke checks updated to the new form.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jagguji
force-pushed
the
feat/155-record-props-flag
branch
from
July 22, 2026 10:13
c451640 to
c4d1f95
Compare
jagguji
added a commit
that referenced
this pull request
Jul 22, 2026
The repo has **9 open CodeQL alerts on `main`** (visible in the default-branch scan). None come from any feature PR — they get re-attributed onto large-regeneration PRs (#153, #156) by CodeQL's "changes too large" re-scan, showing up as a red aggregate **CodeQL** check even though the `Analyze` jobs pass. This clears the root debt so that check goes green. ## Fixes **Code — `js/file-system-race` (high ×3) + `js/identity-replacement` (medium)** - `test/lib/diff.mjs` — `readdirSync(dir, { withFileTypes: true })` instead of `readdir` + a separate `statSync` (the stat is a check-then-read TOCTOU: the file could vanish between the two syscalls). - `src/cli.mjs` — `unlink` directly inside the existing try/catch instead of `existsSync`-then-`unlink` (the check-then-unlink is the race; the catch already handles "already gone"). - `benchmark/run.mjs` — read the prior lock-stamp via try/catch instead of `existsSync`-then-`readFileSync`; and drop the `.replace(/^_/, '_')` in `slugOf`, which was an **identity no-op** (`"@s/p"` → `"_s_p"` already starts with `_`). **Workflows — `actions/missing-workflow-permissions` (medium ×3) + `actions/unpinned-tag` (medium ×2)** - `ci.yml` — add `permissions: contents: read` to the `test` / `compile` / `fixture-guard` jobs (least-privilege; the `preview` job already declared its own). - `benchmark.yml` + `yama-review.yml` — pin `marocchino/sticky-pull-request-comment` and `juspay/yama` to their release commit SHAs, with the `# v3` / `# v2.7.1` version as trailing comments (the CodeQL-recommended form). *Note: `yama-review.yml` is generated by juspay/yama's setup script, so a future regen may need the pin re-applied.* ## Safety All behaviour-preserving — verified: **104 goldens match + compile**, **benchmark byte-identical** (the `slugOf` and lock-stamp changes are proven by the baselines still resolving), and the workflows parse. No `src/` runtime logic changed (the `cli.mjs` edit is the `--clean` stale-file removal, same effect). After this merges, the pre-existing alerts clear repo-wide and the red **CodeQL** check on #156 (and future large-regen PRs) goes green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Closes #155. Amended per review discussion: no flag — record-props is now the only output form.
Problem
Whether a component binds as
type props = {…}+external make: React.component<props>or as classic labeled args was decided solely by "extends*HTMLAttributes/ shares a base" (recordPropsinemit.mjs). A component with a plain, self-contained props object — blend'sAccordionItem— always fell to labeled args and never got a nameablepropstype, which blocks the wrapper/override pattern:Fix — one output form, no flag
Every component with ≥1 prop emits the record form. Deliberately not a flag (the first cut was
--record-propsopt-in; review pushback was right): two output modes double the test surface forever and force every consumer regen script to carry an option — for zero benefit, since the forms are behaviorally identical for JSX (v4 lowers to make/props either way).--record-propsremains accepted as a no-op so any script that adopted it during the preview keeps working.Correctness
propLinerenders both forms; flag markers byte-equal on the benchmark corpus (blend: 910 = 910).type props<'a>+React.component<props<'a>>; this also makes the Catalog ofminorfidelity widenings (TS type vs generated ReScript) for triage — blend 0.0.37-beta.6 #65 variant-fallback path compile-safe (tyvar declared, not free).no-props).{...props, subtext: ?mine}spread-with-override).Verification
record-props(renamed fromrecord-props-flag, no flag in args); smoke checks updated to assert the record form as the default.🤖 Generated with Claude Code