Repository navigation
fix: type nullable TypeScript model properties as T | null - #2023
Merged
Merged
Conversation
The API returns null for nullable response fields such as passwordPwned, but the web, node, react-native and deno models typed them as optional only, so the value was unrepresentable under strictNullChecks. Every other SDK already encodes nullability.
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 #2021
What
The TypeScript SDKs (web, node, react-native, deno) typed
nullable: truemodel properties as optional only (passwordPwned?: boolean). The API documents and returnsnullfor these, so understrictNullChecksthe real value couldn't be represented. Every other SDK already encodes it (Bool?,bool?,*bool,?bool,Optional[bool],Option<bool>).Nullable properties now render as
T | null. Optional non-nullable properties stay?: T.This applies to response models and request models in all four TS SDKs.
How
tsModelPropertytakes anullableflag, so the formatter sees the full type when deciding on line breaks. Generated code has to be Prettier-clean already, and Prettier 3.9.6 lays out an overflowingT | nullin one of three ways. I checked each layout against Prettier directly:T | nullfits on the next line (plain names, arrays and union arrays)web/src/models.ts.twig,react-native/src/models.ts.twigandnode/src/models/requestModel.ts.twigpassproperty | schemaNullable.deno/src/models.d.ts.twigappends| null.Release note
Widening a property's type breaks source compatibility for strict TypeScript consumers whose reads assumed
T | undefined. This should ship in the next major of each TS SDK.Tests
One shared e2e line that every SDK prints:
presetmodel gains an optional nullableverifiedboolean. The mock server'sGET /mock/tests/general/presetreturns it asnull.Base::NULLABLE_MODEL_RESPONSES(preset verified: null) sits next toMODEL_DEFAULT_RESPONSESin every e2e class that uses it (all 33). Each language program prints the decoded value: apple, swift, android, kotlin, dart, flutter, dotnet, unity, go, go-v2, php, python, ruby, rust, node, web (node and chromium), react-native and deno. This also covers decoding of an explicitnullin every SDK.JS_MODEL_TO_STRING_RESPONSESnow includes"verified":null, so the JS SDKs keep the null throughtoString().Models.Preset["verified"]and assignsnullto it, so Deno 1.19 (which type-checks ondeno run) fails to compile without this fix.Verification
npm run format:check,lintandanalysepass on web (client), node (server, includingnpm test) and react-native (client), generated from the live 2.4.x spec. The diff touches 303 node model properties and adds only| null.prettier --check.composer lint,composer refactor:checkandcomposer lint-twigall pass.