fix54035, extractType now allows parts of union and intersection types to be extracted

[iisaduan]

Fixes #54035

[andrewbranch]

Nit: it feels like like a lot of this can probably be deduplicated between the array and non-array cases. Maybe instead of an if/else the newTypeValue/newTypeNode variable could incorporate an isArray(selection) into its assignment. Then, you should be able to use changes.replaceNodeRange in both cases (instead of changes.replaceNode in the else, specifying selection as both the start and end of the range)

[gabritto]

@typescript-bot pack this

[typescript-bot]

Heya @gabritto, I've started to run the tarball bundle task on this PR at 25214a5. You can monitor the build here.

[typescript-bot]

Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158246/artifacts?artifactName=tgz&fileId=757806A4DE6E41A4AD666062F7A45B9502DC0D19C5AC00E33777F87C281E3A6E02&fileName=/typescript-5.3.0-insiders.20231016.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.3.0-pr-56131-2".;

[gabritto]

What happens if the selection doesn't fully enclose the types in a union?
I tried out these cases:

type a = { a: string /*1*/} | { b: string }/*2*/ | { c: string };

and it becomes this:

type NewType = {
  a: string;
};

type a = NewType | { b: string } | { c: string };

and

type a = { a: st/*1*/ring } | { b: string }/*2*/ | { c: string };

which becomes this:

type NewType = string;

type a = { a: NewType } | { b: string } | { c: string };

Those surprised me a bit, but tbh I don't know how much we care about this or how much people are annoyed or not by this behavior.

[andrewbranch]

When @navya9singh implemented partial selection ranges for move to file, I think the logic was basically that the selection shrinks over whitespace and otherwise grows to encompass a valid set of nodes to move. It would be nice to have similar behavior between all selection-based refactors. So for example

type a = { a: string }/*1*/ | { b: string } | /*2*/{ c: string }; // extract b
type a = { a: string /*1*/} | { b: string }/*2*/ | { c: string }; // extract a and b
type a = { a: st/*1*/ring } | { b: string }/*2*/ | { c: string }; // extract a and b
type a = { a: st/*1*/ring }/*2*/ | { b: string } | { c: string }; // extract a
type a = { a: st/*1*/ring /*2*/} | { b: string } | { c: string }; // extract string

is probably what I’d expect

[iisaduan]

The original behavior didn't check for a lot of these imperfect selection cases, and as a result there is some weird behavior. For example

type A = /*1*/{ a: string } & { b: string } & { c: string }/*2*/ ;  // extracts a, b, c
type A = {/*1*/ a: string } & { b: string } & { c: string }/*2*/ ;  // extracts a
type A = { a: string } /*1*/& { b: string } & { c: string } ;/*2*/  // extracts a, b, c
type A = { a: st/*1*/ring }/*2*.; // extracts string

are all things that happen with the original behavior.

Those surprised me a bit, but tbh I don't know how much we care about this or how much people are annoyed or not by this behavior.

It would be nice to have similar behavior between all selection-based refactors.

I as well was surprised by this behavior, but I was also surprised that this partial type extraction bug wasn't noticed sooner. I think it would be really nice to have the behavior do be less finicky for the user and be more consistent across refactors, but again, not sure how much it's been noticed.

[gabritto]

We might want to check with Matt, since he opened the original issue, to get a sense of how much this partial selection behavior matters.

[iisaduan]

After discussion with Matt today, and taking another look at the code that picks the nodes from the selection, adding small improvements such as

// type A = { a: str/*1*/ing } | { b: string } | { c: string }/*2*/ ;  // extracts string

becoming

// type A = { a: str/*1*/ing } | { b: string } | { c: string }/*2*/ ;  // extracts a, b, c

will require adjustments to the way I did the bug fix here. The existing code assumes the start of the selection is well placed, and my fix uses that assumption.

[iisaduan]

The most recent change only addresses the behavior discussed above, and doesn't require such a big change like I had originally expected.

[gabritto]

Left one small question, but otherwise looks good to me

[gabritto]

I'm not sure I understand what's going on here: if visitor returns true, that means we want to skip collection of type parameters and so we want collectTypeParameters to return undefined, right?
It seems to me we're not doing that anymore here when selection is an array, since inside this if we're always returning the result array.