Skip to content

Give function type parameters descriptive names - #2173

Merged
robhogan merged 1 commit into
mainfrom
pr2173
Nov 5, 2026
Merged

robhogan merged 1 commit into
mainfrom
pr2173

Conversation

@robhogan

@robhogan robhogan commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Follow up to #2169, which named every unnamed parameter in Flow function types v, v<index> or rest. This replaces those placeholders with descriptive names, taken from the implementation or call site wherever there is one.

The only change beyond names is dropping two void parameters that are never passed, on resolve in IncrementalBundler and cb in bundle.flow.js.

Changelog: Internal

Test plan:
Compiled each changed file outside flow-typed/ with the repo's Babel config before and after this change, and the output is identical. API snapshots are unchanged.

Follow up to #2169, which named every unnamed parameter in Flow function types `v`, `v<index>` or `rest`. This replaces those placeholders with descriptive names, taken from the implementation or call site wherever there is one.

The only change beyond names is dropping two `void` parameters that are never passed, on `resolve` in `IncrementalBundler` and `cb` in `bundle.flow.js`.

Changelog: Internal

Test plan:
Compiled each changed file outside `flow-typed/` with the repo's Babel config before and after this change, and the output is identical. API snapshots are unchanged.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 4, 2026
@robhogan
robhogan requested a review from SamChou19815 November 4, 2026 21:12
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Metro review: Looks good

Risk: low · Commit: 0f7e01d · 8 commands run

This PR replaces the v, v<index> and rest placeholders in Flow function types with descriptive names and drops two unused void parameters. Its claims hold: compiled output is identical across all 54 non-flow-typed JS files, and with parameter names erased, ASTs across all 61 changed files differ only in those two void parameters. Flow, TypeScript, lint, formatting and API snapshots pass, and the names I checked against implementations and call sites match, so I have no findings.

No findings.

Verified
  • ✅ yarn typecheck — Flow reports no errors on head.
  • ✅ yarn typecheck-ts — Passes on head and base. Includes TypeScript definition generation.
  • ✅ yarn verify-api-snapshots — 18 snapshots verified, with no API.md changes in the PR.
  • ✅ yarn lint
  • ✅ yarn format-check
  • ✅ node compare-babel.js — Compiled the 54 changed JS files outside flow-typed/ from both checkouts with the repo's Babel config: 0 differ.
  • ✅ node compare-ast.js — Parsed all 61 changed files with parameter names erased. Only IncrementalBundler.js and bundle.flow.js differ, at the two dropped void parameters. Reports the first difference per file.
  • ✅ yarn jest --no-watchman <changed metro-file-map, HMRClient, Graph and HmrServer tests, plus api-snapshots-sync-test> — 17 suites, 656 tests. Some changed test files were not in this subset.

Not run: No benchmarks: compiled output is byte-identical, so runtime cost can't change. The full Jest suite wasn't run, for the same reason. Windows wasn't tested because the change only touches erased types and no Windows machine was available. An initial combined lint and snapshot command was denied by the session's permission check because it used a shell variable, so those checks were rerun as separate commands: repo-wide yarn lint and yarn format-check, and yarn verify-api-snapshots.

Automated review by Claude. Advisory only: it never approves or blocks, and a maintainer makes the call.

@robhogan
robhogan merged commit b54f193 into main Nov 5, 2026
16 checks passed
@SamChou19815
SamChou19815 deleted the pr2173 branch November 5, 2026 13:23
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 Facebook bot. Authors need to sign the CLA before a PR can be reviewed. needs-meta-import

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants