TS4.4 exactOptionalPropertyTypes: options#3959
TS4.4 exactOptionalPropertyTypes: options#3959quisido wants to merge 1 commit intogetsentry:masterfrom quisido:patch-2
Conversation
|
Hey, thanks for the PR, and TIL about this flag from 4.4. I do agree that in most cases it is indeed permissible in our codebase to pass Once we allow for passing I'll try to discuss this with the team soon, so I'll leave this PR without any changes for now. Cheers! |
|
I think the idea is you definitely do want to support undefined, with no going back. The idea of introducing this flag is to distinguish If Sentry is not relying on the keys existing, you would want the explicit use of undefined here so that both styles pass transpilation, since both styles produce the same runtime behavior. For an example of how one must interact with Sentry with optional properties, and why I opened this PR, see my code here: The whole checks and optional setting could be so much simpler as just |
|
@CharlesStover I cherry-picked your commits and created a joined PR + updated |
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).I cannot lint in this instance, because I am PR'ing from the GitHub web UI. 🙃
TypeScript 4.4 added a new compiler option,
exactOptionalPropertyTypes, which distinguishes between{}and{ key: undefined }. With this flag,f(key?: string)may either bef()orf('str'), but may not bef(undefined). In most cases,f(undefined)should be acceptable, including in Sentry.Example:
In the above example, TypeScript 4.4 throws an error with the flag because
allowUrlsmust either be an array or not set at all. It does not allow this code such that it is set toundefined.For future consumer-facing code, please replace
[key]?: Twith[key]?: T | undefined. 🙃Sorry in advance that each file is a separate PR. This also, however, allows blast radius to be reduced, and is particularly relevant for any properties where
undefinedshould not be supported.