GeezerVison

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping Sheetal Nandi (@sheetalkamat), Andrew Casey (@amcasey), Matt Bierner (@mjbvz), Mine Starks (@minestarks) for you. Feel free to loop in other consumers/maintainers if necessary

Protocol changes look good. I'll give it another quick test locally and let you know if I run into an big issues

Looks great. Just found a few odd results that cause VS Code to error:

export function loadRepos() {
  return {
    type: /**/LOAD_REPOS,
  };
}

Top level returned text span is:

        "textSpan": {
            "start": {
                "line": 1,
                "offset": 27
            },
            "end": {
                "line": 1,
                "offset": 27
            }
        },

describe('thing', () => {
    it('/**/', () => {});
});

Returned span is:

        "textSpan": {
            "start": {
                "line": 1,
                "offset": 20
            },
            "end": {
                "line": 1,
                "offset": 20
            }
        },

class HomePage {
    componentDidMount() {
        if (this.props.username/**/) { }
    }
}

Returned top level span is

 "textSpan": {
            "start": {
                "line": 2,
                "offset": 23
            },
            "end": {
                "line": 2,
                "offset": 23
            }
        },

Hm, I can see why the empty string one is doing that, but the others seem odd. I'll check and see what it thinks it's doing, but it looks like I just need to add a filter to reject zero-width spans. Will update first thing tomorrow. Thanks for testing!

Ohhh. Those really strange ones were caused by a possible bug in ts.positionBelongsToNode that Ryan Cavanaugh (@RyanCavanaugh) and Ron Buckton (@rbuckton) and I were discussing yesterday. I've since switched away from using that API, and just added a filter to ignore empty spans.

I can no longer repro the bugs you gave me, but I thought I stopped using ts.positionBelongsToNode before you tried it yesterday... I just added unit tests for those cases but I'll try the integration with your custom VS Code build too.

I agree the fourslash format looks like a disaster.

The motivation, though, is that in general position-based tests end up being a lot less readable and maintainable than DSL-based formats, so we try to avoid the former as much as possible.

It's impossible to look at these tests right now in the diff view, for example, and understand what the expected behavior is, and if something changes in the wire format, there's a bunch of updating that will have to happen. Updating the text is also very difficult - adding a single character in the middle of an existing test is going to require a lot of tedious math.

Having a one-off DSL has generally paid for itself in terms of long-term test maintenance. For example, we could imagine having something like this (bikeshed a bunch):

const test = `const { x, y: a, ...zs = {} } = {};`;
const expected = [
    `const { x, y: a, ...zs = {} } = {};`,
    `             [a]                    `,
    `          [y: a]                   `,
    `     [{ x, y: a, ...zs = {} }]     `,
];

Alternatively, this might be an appropriate place to use baselines that just render a human-readable format of what happened. Writing a one-off renderer to make an inspectable text file for what the selection spans were would be pretty easy, and lends itself to reviewers being able to see what the behavior is.

👍🏽 I like the idea of a baseline. The assertions are difficult to write no matter what, but there are formats that are easy to read—eliminating the writing step seems like the best approach.

Ryan just gave me a physical thumbs up on this in the room because his computer currently has a blue screen of death 😄

Andrew Branch (@andrewbranch) yey on first big feature merge 👍

External observer question: why is selectionRange-full is needed?

   export interface TextSpan {
      start: Location;
      end: Location;
  }

. . .

Also exposes a selectionRange-full tsserver command which accepts the same request format but responds with the language service response unchanged, which is the same except that TextSpan is a ts.TextSpan:

 export interface TextSpan {
     start: number;
     length: number;
 }

Given general brevity of the docs on ts APIs, adding odd-shaped nearly-identical calls can easily lead people astray.

Good question. Almost every TSServer command has a -full counterpart that gets used by Visual Studio. I believe Visual Studio internally uses 0-based lines and columns, while I know VS Code uses 1-based lines and columns.

I actually wrote up something about this for the wiki, I just haven’t added it yet.

Sorry, I didn't realise it's already consistent with other calls. Makes total sense!

I believe the primary difference between the -full and regular versions is that -full uses a character offset from the beginning of the file, rather than line and column.

Yep, that’s right, and that’s the same way that the compiler itself typically represents positions in files. Maybe Visual Studio doesn't use lines and columns much internally at all ¯_(ツ)_/¯