fix(types): accept an explicit undefined on every prop this package adds - #418
Open
YevheniiKotyrlo wants to merge 2 commits into
Open
fix(types): accept an explicit undefined on every prop this package adds#418YevheniiKotyrlo wants to merge 2 commits into
YevheniiKotyrlo wants to merge 2 commits into
Conversation
Under `exactOptionalPropertyTypes`, a bare `?: string` FORBIDS passing an
explicit `undefined` — and `className={condition ? 'a' : undefined}` is
exactly what a conditional class spreads. So every prop this package adds to
React Native's own interfaces rejected a value React Native's own optional
props accept, at every such call site.
Each added declaration is widened to `| undefined`. No runtime behaviour
changes; this is the declaration matching what the implementation already
allowed.
React Native's own optional props are declared `| undefined`; the props this
package augments them with are not. Under `exactOptionalPropertyTypes` that makes
`className={condition ? "p-4" : undefined}` — the ordinary conditional — a type
error, and there is no way for a consumer to fix it except by patching the
package.
`src/runtime.types.ts` needs the same treatment. Its three mapped types re-narrow
className to a bare optional, so `react-native-css/components` and `styled()` —
the imports the README documents — stay broken even once `types.d.ts` is fixed.
The contract is machine-checked. `src/__tests__/types` is a two-file program that
compiles the fixture against `types.d.ts` with the flag on; reverting `types.d.ts`
turns it red with 15 errors while the root typecheck stays green, which is the
point — the existing typecheck structurally cannot observe this.
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.
Problem
React Native declares its own optional props as
| undefined— 698 of them, and none bare. The props this package augments them with are bare?: string. UnderexactOptionalPropertyTypesthose two forms are not the same, and the difference falls on the most ordinary thing a consumer writes:Eleven authoring patterns fail this way — the conditional above, a
string | undefinedvariable, a spread bag, a wrapper forwarding its own optionalclassName, and the same forcssInterop,contentContainerClassName,indicatorClassName,placeholderClassName. A consumer cannot fix any of it locally: declaration merging can add props but never re-declare the optionality of existing ones. The only escape is patching the package or writing{...(cond ? {className: x} : {})}at every call site.Fix
Every added prop takes
| undefined, in both places the contract is expressed.types.d.tsis the obvious one.src/runtime.types.tsis the one that is easy to miss:StyledProps,StyledReactElementandStyledComponentsynthesise the same props through mapped types ending]?: string, so they re-narrow it. Without those three tokens the bug still reproduces onreact-native-css/componentsandstyled()— the imports the README documents.There is one observable change beyond permissiveness, and it goes toward consistency rather than away: under the flag,
Required<ViewProps>['className']previously strippedundefinedwhileRequired<ViewProps>['style']kept it. Now they agree.Tests
src/__tests__/types/is a two-file program — the fixture plustypes.d.ts— compiled withexactOptionalPropertyTypeson, wired intotypecheckas a secondtscinvocation.src/__tests__/babel/tsconfig.jsonis the existing precedent for a nested tsconfig here, and the leading underscore keeps the fixture out of jest via the existingtestPathIgnorePatterns.It is mutation-proven: reverting
types.d.tsturns the gate red with 15 errors while the roottypecheckstays green. That gap is the reason the gate exists — with the flag off, the old and new declarations are the same type, so no fixture compiled by the currenttypecheckcan tell them apart. I checked the alternative first: a@ts-expect-errorfixture is worse than useless, because it passes only in the broken configuration and turns CI red once the types are fixed.The gate deliberately does not cover
runtime.types.ts— importingStyledPropspulls 22 source files into the program and surfaces nine pre-existing errors unrelated to this change. Enabling the flag repo-wide is a reasonable follow-up (22 errors across 13 files today) but a different PR.npm pack --dry-runships no__tests__;yarn buildis unaffected, since bob's babel targets exclude__tests__and its typescript target reads only the root tsconfig. One caveat:lefthook.yml's pre-commit runs bareyarn tsc, so the gate runs in CI but not on a local commit.Full suite, typecheck and lint measured against a pristine-
mainbaseline on the same machine — no new failures.Notes
example/example-env.d.tsis left alone deliberately. It is a generated duplicate, already divergent fromtypes.d.tsonmain, and referenced by nothing.types.d.tsinternally consistent.CONTRIBUTING.mdasks that API changes start as an issue. Published types are API, so tell me if you would rather I open one — I led with the compile evidence because the change is small and the failure is mechanical.Scope note. This sweep covers every prop declared in
types.d.tsas of this branch. #416 addsa
react-native-gesture-handlermodule augmentation to the same file; its two props are alreadydeclared
| undefined, so the invariant holds across both, but the two PRs touch overlappinglines and whichever lands second needs a rebase.