Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring to convert && chain to optional chain expression #35018

Closed
DanielRosenwasser opened this issue Nov 9, 2019 · 15 comments · Fixed by #39135
Closed

Refactoring to convert && chain to optional chain expression #35018

DanielRosenwasser opened this issue Nov 9, 2019 · 15 comments · Fixed by #39135
Assignees
Labels
Committed The team has roadmapped this issue Domain: Refactorings e.g. extract to constant or function, rename symbol Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

interface Foo {
    bar?: {
        baz?: string;
    }
}

declare let foo: Foo | undefined;

[|foo && foo.bar && foo.bar.baz|]

It should be possible to convert that chain into the following:

interface Foo {
    bar?: {
        baz?: string;
    }
}

declare let foo: Foo | undefined;

foo?.bar?.baz;
@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Domain: Refactorings e.g. extract to constant or function, rename symbol labels Nov 9, 2019
@DanielRosenwasser DanielRosenwasser added this to the Backlog milestone Nov 9, 2019
@DanielRosenwasser DanielRosenwasser self-assigned this Nov 9, 2019
@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Nov 9, 2019

You know, mostly like this

convertToOptionalChain

@Urigo
Copy link

Urigo commented Nov 9, 2019

maybe a code-mode or babel transform that runs throughout a codebase?

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Nov 9, 2019

@Urigo That could be done too, but I think having this granularity is good because it's not strictly the same semantics. ?. always returns undefined, && keeps the original nullish value.

@switz
Copy link

switz commented Nov 10, 2019

Also keep in mind that optional chaining returns true on 0 and '' (empty string), normally falsy values in javascript. So running this as a code mod would almost certainly cause unintended bugs.

@MLoughry
Copy link

Also keep in mind that optional chaining returns true on 0 and '' (empty string), normally falsy values in javascript. So running this as a code mod would almost certainly cause unintended bugs.

I'm not sure I really follow. Such code would be rather questionable in the first place, no? If you have an && chain that could be converted to an optional chain, and it returns '' or false as a short-circuit, then the same code would be throwing a TypeError if the value is a non-empty string or true.

@fatcerberus
Copy link

I could certainly envision a scenario where code was written that depends on short-circuiting on falsy values. We're eternally stuck with typeof null === 'object' for backward compatibility reasons, after all.

Also there's no TypeError because boolean and string are object-coercible. The subsequent property access would just return undefined and short-circuit.

@jineshshah36
Copy link

[|foo && foo.bar && foo.bar.baz|]

What do the “|” mean?

@dragomirtitian
Copy link
Contributor

@jineshshah36 [|code|] is the syntax used typescript four-slash tests to mark a selection. Some diagnostic suggestions are not exposed unless they can be applied to the selected code.

@MrAndersen1
Copy link

All too eager to start using the ?. syntax I managed to play a trick on myself by converting
if (foo && foo.bar && foo.bar.indexOf(someValue) !== -1)
to
if (foo?.bar?.indexOf(someValue) !== -1)
which had quite the opposite behaviour of what was intended heh.

Just one of the (probably more obvious in hindsight) things a script would have to keep in mind :)

@DanielRosenwasser DanielRosenwasser changed the title Convert && chain to optional chain expression Refactoring to convert && chain to optional chain expression Apr 22, 2020
@RyanCavanaugh RyanCavanaugh modified the milestones: Backlog, TypeScript 4.0 May 4, 2020
@RyanCavanaugh RyanCavanaugh added the Committed The team has roadmapped this issue label May 4, 2020
@DanielRosenwasser
Copy link
Member Author

There's a question of whether or not nullish coalescing (??) should be supported here as part of the code action.

@DanielRosenwasser
Copy link
Member Author

Also, what the supported patterns are:

Category Input Output Comments
&& property chains: a && a.b.c && a.b.c.d a?.b.c?.d
Ternary checks with property access a.b ? a.b.c : "hello" a.b?.c ?? "hello" Is this one type-driven? What if c is string | null?
&& property/call chains: a && a.b.c && a.b.c() a?.b.c?.()

@Kingwl
Copy link
Contributor

Kingwl commented May 13, 2020

Ahhhh, i have some existed work about this. And I'm happy to port here

@kylemh
Copy link

kylemh commented May 13, 2020

I’m not sure of the viability of going after library refactorings (as opposed to just JS -> TS), but I think one huge win would be if TS could automatically refactor lodash/get usages and/or ramda/pathOr usages.

@Kingwl
Copy link
Contributor

Kingwl commented May 13, 2020

if TS could automatically refactor lodash/get usages and/or ramda/pathOr usages.

Good idea. IMO TypeScript cannot provide that but there's outside tools.
eg: https://github.com/HearTao/ts-upgrade

@Kingwl
Copy link
Contributor

Kingwl commented May 20, 2020

Are there any existed tools or algorithms to compare expression and judge they are equality?
I have some ugly work about it but they cannot work well.

@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Domain: Refactorings e.g. extract to constant or function, rename symbol Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.