Skip to content

EPBDS-16211: fix the findings the TypeScript migration left open - #148

Merged
AlexSamBY merged 1 commit into
masterfrom
fix-findings
Oct 1, 2026
Merged

AlexSamBY merged 1 commit into
masterfrom
fix-findings

Conversation

@AlexSamBY

Copy link
Copy Markdown
Member

What changes

The findings the TypeScript migration left open, fixed in one PR. Each was recorded in docs/UPGRADE-PLAN.md as found and left for a change of its own. Each record is now marked with its outcome.

Bugs a host could see

A float Dropdown keeps its label above the selected value.

  • The wrapper computed done from props.value. But value had already been taken out of the rest bag, so props.value was always undefined, and done was always false.
  • So the stylesheet's completed state (.input--wrapper.done in input.less) never applied:
    • the label of a float dropdown fell back over the selection it named;
    • a filled multiple selection kept its border.
  • done now comes from the value the parent controls. An empty selection, an empty string, null, and a field with an error are still not completed; 0 is a value.
  • The bug predates the modernization: the code came in with it.
  • New suite, Dropdown.done-state.test.js: 2 of its 8 tests fail on the previous code. The rest pin the boundaries.
  • jsdom cannot lay out, so the tests assert the class the stylesheet selects on, not the label's position. No tracked meta has a float dropdown.
  • The full-DOM snapshots were regenerated for the 5 examples that changed. The diff is 6 lines, and in each one a dropdown wrapper gains done. The contract's header records the regeneration, as it asks. The same snapshot holds on React 16, 17, 18 and 19.
  • One existing test pinned the bug, and it is rewritten. Dropdown.test.js said "does NOT auto-set done class in standalone mode", and its comment claimed that a form wrapper supplies props.value. It does not: measured across the corpus, no dropdown inside a form was done either.

validate: 'password' no longer throws on a server without a strength checker.

  • In a browser, Active.passwordCheck falls back to "skip" until zxcvbn loads.
  • On a server it returns what the host assigned, and undefined otherwise. _envs pins that undefined, and the rule called it anyway, throwing TypeError: Active.passwordCheck is not a function.
  • passStrength now skips the check when no checker is configured. The registry's contract is left as it was.
  • Reproduced on master first. A server render did not throw, because validators do not run there; any validation in Node did.
  • New validationRules.server.test.js (node environment): 1 of its 3 tests fails on the previous code.

The validation-errors summary is headed "Please complete:".

  • validationErrorsTooltip rendered "Untranslated" above the list. The phrase lived in form/translations.js, which nothing imported, and E2 deleted that file.
  • It is now registered beside its one reader, as the components register theirs.
  • New test in utils.field-lifecycle.test.js; it fails on the previous code.

A Table with colGroup no longer logs React's missing-key warning.

  • Each <col> gets its position as its key; the columns are the meta's, in its order.
  • The new test has a file of its own: React warns once per component and module, so a test placed after another render would pass whatever the code does.

Dead code and dead checks

  • The options popupOpen handed popupAlert are gone. Every path passed a popup's own props merged with the options it was opened with. No version of popupAlert ever read them; the first, in 2020, took (title, content) as this one does. Behaviour is unchanged. docs/SUPPORTED-VIEWS.md now says what a popup reads (title and items) and which popupOpen options are read (relativeIndex and relativePath).
  • Five utils exports with no caller are deleted, with their tests: optionsFrom, enumFrom, localise, fileFormatNormalized and LANGUAGE_LEVEL. Only their own tests used them; the library exports nothing but the component.
  • The examples suite allows no console error at all. Its allowlist had five entries left. Measured on React 16.14, 17.0.2, 18.3.1 and 19.3.0, none matched anything. An allowlist that matches nothing hides the next real warning of the same shape.

static/images/flags/ is no longer shipped

This is the maintainers' decision.

  • The folder was 266 of the tarball's 289 files and 41% of its unpacked size, and nothing in the library read it.
  • A host that links a flag from its own markup or a meta Image now ships its own copy. The changelog and the README say so.
  • The copy rule is removed, and pl.svg leaves the required files.
  • The budgets are lowered to the new baseline. The old limits would not have noticed the folder coming back.
  • check-package-budget.js gains MUST_NOT_SHIP. It fails a tarball that carries the folder again, by name. I proved it with a flag put back into static/.
  • The source files stay in public/, which is the demo's.

The engine is held to the published host interface (item 9)

The host's props reach the engine through one cast in main.tsx. So nothing checked that what the engine calls a host with, and what it hands a host, is what contract.ts publishes. contract.agreement.ts now asserts it, and it found two disagreements:

  • downloadFile: the engine typed the host's answer as a whole fetch Response. The contract promises blob(), which is all the engine reads, and the engine now asks for exactly that.
  • The onError report: React's ErrorInfo types componentStack as possibly missing (@types/react 18 and 19), and the contract promises a string. The boundary now passes it on as a string, '' should React give none. A new test calls the boundary directly with no stack, because no render makes React omit one; it fails on the previous code.

There are five assertions: downloadFile, uploadFile, updateExperienceData, the error report, and the meta problem. I broke each one on purpose, and each failed at its own line.

Found, and left open

getValidationErrors hands a host each field's error as the validator returned it, typed text: unknown, while the contract publishes text: string. A custom validator that returns an object reaches the host as an object. This needs your decision rather than a fix:

  • converting the error would change what hosts receive;
  • loosening the contract would break a host that reads text as a string.

Measured

master this PR
corpus, 38 examples: commits at mount / translate calls 107 / 1012 107 / 1012
on an edit: commits / translate calls 27 / 64 27 / 64
DOM after mount 6 dropdown wrappers in 5 examples gain done; nothing else differs
DOM after an edit identical, all 38
tarball 289 files, 5.6 MB unpacked, 1.4 MB packed 23 files, 3.3 MB unpacked, 0.85 MB packed
dist/index.js 311,535 bytes 311,559 bytes

The 24 bytes are the registered phrase and the stack normalization, less the dead popup argument. The deleted utils exports were never in the bundle. Both bundles were built from git archive copies in directories of the same name length.

Gates

All exited 0 on the second full run, on this branch:

  • lint:js, lint:css, typecheck, typecheck:contract;
  • docs:props:check, docs:views:check, css:fixture:check;
  • test:env-flags, build;
  • test:coverage, thresholds included (Render.tsx, Dropdown.tsx, definitions.ts stay at 100%);
  • test:pack, test:pack:peers, test:types;
  • test:react16, test:react17, test:react19, 2854 tests each, with no worker crash;
  • test:e2e, 41 tests;
  • the manifest contract.

The first full run is not counted, and here is why. It ran before the DOM snapshots and the old Dropdown test were updated, so the same 6 tests failed on every leg. On React 16 a jest worker also died with SIGSEGV in rules.popup-actions.test.js, which is the known V8 crash (nodejs/node#62393). The second run, of everything, had no crash.

The suite went from 2855 to 2854 tests: 14 new ones, and 15 that tested the deleted utils exports.

🤖 Generated with Claude Code

Each of these was recorded as found during the migration and left for a
change of its own.

- A float Dropdown keeps its label above the selected value. The
  wrapper read props.value after taking value out of the rest bag, so
  `done` was always false and the stylesheet's completed state never
  applied.
- validate: 'password' no longer throws on a server that configured no
  strength checker. The check is skipped there, as in a browser where
  zxcvbn has not loaded.
- The validation-errors summary is headed "Please complete:". The
  phrase lived in a file nothing imported, so the header always read
  "Untranslated".
- Each <col> of a Table's colGroup has a key, so React logs no warning.
- The options popupOpen passed to popupAlert are gone. No version of
  popupAlert ever read them; the view reference now says what a popup
  and popupOpen read.
- Five utils exports with no caller are deleted, with their tests:
  optionsFrom, enumFrom, localise, fileFormatNormalized and
  LANGUAGE_LEVEL.
- The examples suite allows no console error at all. Its allowlist
  matched nothing on React 16, 17, 18 or 19.
- The package no longer ships static/images/flags/, as the maintainers
  decided. The budgets are lowered to match, and the budget check fails
  a tarball that carries the folder again.
- The engine is held to the published host interface. downloadFile
  asked for a whole fetch Response where the contract promises blob(),
  and the onError report could carry a missing componentStack where
  the contract promises a string. Five type assertions now cover the
  host calls and what the engine hands a host.

Found, and left open: getValidationErrors hands a host each error as
the validator returned it, while the contract promises a string.

Measured: commits and translate calls are identical across the corpus.
The DOM differs only where it should: 6 dropdown wrappers in 5 examples
gained `done`. Their full-DOM snapshots are regenerated, and that is
the whole diff. An old Dropdown test that pinned the bug is rewritten.
The tarball went from 289 files, 5.6 MB unpacked, to 23 files and
3.3 MB.
@AlexSamBY
AlexSamBY merged commit d931c72 into master Oct 1, 2026
9 of 10 checks passed
@AlexSamBY
AlexSamBY deleted the fix-findings branch October 1, 2026 13:53
AlexSamBY added a commit that referenced this pull request Oct 1, 2026
## What changes

**`examples.strict-mode.test.js` now compares its two runs on virtual
time.** Before, a slow machine could make the two runs read different
moments of the same animation.

### The failure

[CI run
36870778501](https://github.com/eisgroup/ui-render/actions/runs/36870778501/job/110397669110)
(#148), job `react-16-floor`, failed once in "every example under
StrictMode › all renders, and reports, what it does without it". A rerun
of the same job passed.
- **The two DOM strings differed in one attribute.** It was
ProgressBar's fill: `style="width: 0%;"` without StrictMode,
`style="width: 100%;"` with it. All 253 class attributes were the same.
- **ProgressBar mounts empty and fills its bar 200 ms later**
(`TIME_DURATION_INSTANT`), from an effect's `setTimeout`.
- **The test read the DOM after a 0 ms flush.** Locally `all` mounts in
12–57 ms on React 16 and 18, so the bar always read 0%. On that runner,
the strict run took longer than 200 ms between the effect and the read.

### Measured before choosing a fix

For every example, I compared the DOM at the moment the test reads it,
250 ms later, and once settled; after an edit, at 400 ms and once
settled.
- **Only `all` still changes after the read.**
- **The only thing that changes in it is that fill**, between 150 and
250 ms after the mount.
- `all` has no text box, so the test's second read follows the first at
once, and both reads were exposed.

### The fix

`exercise()` fakes the timers for the runs it compares, and only the
timers.
- **`setTimeout`/`setInterval` run on a virtual clock,** advanced with
`jest.advanceTimersByTimeAsync`, which lets promises settle between
timers as real time does.
- **`Date`, the microtask queues, `setImmediate` and the animation
frames stay real.** React's act and its scheduler use them, and nothing
in the corpus needs them faked.
- **The read points are the same as before:** 0 ms after the mount, 400
ms after the edit, and 0 ms after the unmount. They are now on the run's
own clock, whatever the machine's speed.

I considered waiting past the fill before the read instead. It would add
about 300 ms to each of the 76 runs, and it would stay a race against
any later timer. I did not mask the `style` attribute; the comparison
gates exactly what it did.

### Proof

- **No change in what is compared.** Real time and virtual time read the
same mounted DOM, the same edited DOM and the same console output, for
all 76 runs (38 examples × 2 modes), on React 16, 17, 18 and 19.
- **The race is reproduced and closed.** I added a probe that held the
thread for 250 ms after the strict mount, as a slow runner does. The old
test failed on `all` on React 16 and 18, with the same `0%` / `100%`
difference as CI. The new test passed on both. The probe is removed.
- **It still catches StrictMode bugs.** I put back the dropdown mount
flag that this test found when it was written. The new test failed on 7
examples, the number the plan records.
- **It is faster.** The suite takes about 1.6 s a leg instead of about
8, because the 400 ms waits after an edit are virtual now.

`docs/UPGRADE-PLAN.md` notes this beside the test's acceptance record
(§9.3 step 7).

## Gates

All exited 0, run on this branch:
- `lint:js`, `lint:css`, `typecheck`, `typecheck:contract`;
- `docs:props:check`, `docs:views:check`, `css:fixture:check`;
- `test:env-flags`, `build`;
- `test:coverage`, thresholds included;
- `test:pack`, `test:pack:peers`, `test:types`;
- `test:react16`, `test:react17`, `test:react19`, 2854 tests each, with
no worker crash;
- `test:e2e`, 41 tests;
- the manifest contract.

The number of tests is unchanged.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant