Repository navigation
Provide upgrade tooling for --strictOptionalProperties #44419
Description
Background
For TypeScript 4.4, we anticipate that users using --strictOptionalProperties will have fairly mechanical transformations on individual properties. For example
interface Options {
maxRetries?: number;
headers?: KnownHeaders & Record<string, string>;
callback?: (path: string) => void;
}probably should just get rewritten to
interface Options {
maxRetries?: number | undefined;
headers?: (KnownHeaders & Record<string, string>) | undefined;
callback?: ((path: string) => void) | undefined;
}We'd like to provide some basic tooling to run through a project's declarations to ensure that properties are "upgraded" appropriately.
Basic constraints
- We should not introduce syntax errors in the generated code.
- We should ensure that the generated code's formatting is reasonable in 90% of cases, and does not drastically change the formatting of the original file.
- We should have an easy way to point the tool at a
tsconfig.json, or a set of understood files to make this work correctly. - We should not run through
node_modules,lib.d.ts, or other files that are outside the control of a user.
Open questions
- What should the tool use? ts-morph? something else?
- How should we distribute this tool? As something standalone, or as a plugin for a broader refactoring tool?
Activity
orta commentedon Jul 2, 2021
Nathan Shively-Sanders (@sandersn) made a tslint rule with a
--fixfor DT in microsoft/dtslint#335, could we recommend or upstream this (and/or) do the same for eslint?a-tarasyuk commentedon Jul 2, 2021
Orta Therox (@orta) I think many projects use
typescript-eslint, so I supposeno-redundant-undefinedshould be there too (I can work on this if needed). What do you think?sandersn commentedon Jul 2, 2021
Like I did in microsoft/dtslint#335, I would call the rule
redundant-undefined, since it should be configurable, and behave differently for parameters and properties.OliverJAsh commentedon Jul 6, 2021
I noticed a bunch of DefinitelyTyped PRs to add
undefinedto optional properties. I'm curious what this change will mean for React, specifically props likeclassName: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/46c3dc08d8efb51686530f24cea216ab8c17d8be/types/react/index.d.ts#L1823className={undefined}should not be allowed as this will produce HTML such asclassName="undefined". /cc Nathan Shively-Sanders (@sandersn)orta commentedon Jul 6, 2021
The key with these automated PRs is that they are 'backwards' compatible in the sense that 4.4 behavior with
exactOptionalPropertyTypesenabled is gives you the same type system behavior as 4.3 without it. As it's 'legal' in TS to passundefinedtoclassNamein 4.3 strict.However, post the PRs it now easy to switch the semantics to be more accurate on a case by case basis, in comparison to before where support for
exactOptionalPropertyTypeswould be very unreliable with modules on DT (because few modules would opt-in to the more accurate syntax)So for React, that
classNamechange can have theundefinedremoved from thestring | undefinedtype after the automated PR happenssandersn commentedon Jul 6, 2021
Quick notes from the tslint rule
redundant-undefinedI upgraded in microsoft/dtslint#335, if you want to use that as a basis for an eslint rule:anyand unions withanysince that fails another DT lint rule.| undefinedafter a union is 90% reasonable. Of course, the 80% case is a single type and 90% just adds simple unions. But I think even more complex types of the last 10% look OK most of the time.a-tarasyuk commentedon Jul 10, 2021
eslint implementation of microsoft/dtslint#335
andrewbranch commentedon Aug 20, 2021
As discussed in #45032, using a TS codefix across a whole codebase is a poor migration strategy for
--exactOptionalPropertyTypes. We feel like Oleksandr Tarasiuk (@a-tarasyuk)’s eslint rule should be good for a bulk migration, while Nathan Shively-Sanders (@sandersn)’s codefix in #45032 can be useful for one-off fixes in your editor. Isabel Duan (@iisaduan)’s codefix runner should be releasable pretty soon and produces great results for plenty of other codefixes, but we don’t recommend it for--exactOptionalPropertyTypes.NickGerleman commentedon Mar 3, 2023
I've been curious about the comments on it not being a good migration strategy. The strategy was applied to large projects of DefinitelyTyped in a single go, including React Native in DefinitelyTyped/DefinitelyTyped@694c663 before we brought the typings into our own package.
It has since been applied inconsistently, and making matters more confusing, our actual source is Flow, which does allow passing undefined. So, to not be more strict than the source, we need to add this. But it is no guarantee it actually works.
Since all of the properties are already modded to allow explicit undefined, I did add the ESLint rule for now.