Adds linked editing for JSX tags by iisaduan · Pull Request #53284 · microsoft/TypeScript

Isabel Duan (iisaduan) · 2023-03-16T06:58:49Z

Adds linked editing of JSX tags (mirror cursor)

TypeScript Bot (typescript-bot) · 2023-03-16T06:58:53Z

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), Matt Bierner (@mjbvz), Kat Marchán (@zkat), and Joaquin Jares (@joj) for you. Feel free to loop in other consumers/maintainers if necessary

TypeScript Bot (typescript-bot) · 2023-03-16T06:58:56Z

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src/lib' or possibly our lib generator. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src/lib' or https://github.com/microsoft/TypeScript-DOM-lib-generator

…mirror

Daniel Rosenwasser (DanielRosenwasser)

Looks like this is all coming together pretty nicely. I left a bit of feedback about

why things weren't working correctly with fragments,
some ideas for how fourslash testing can be made easier
some thoughts on the API (TextSpan vs. TextRange - maybe we revisit that?)
some style feedback

With regards to style, at some point we should consider getting an automatic formatter - but in the meantime, try to stay consistent with the rest of the codebase, and use the editor's built-in formatter as well.

Isabel Duan (iisaduan) · 2023-03-31T22:04:38Z

I've renamed the feature to linkedEditing..., to match the LSP and allow for potential non-JSX tag usage in the future.

Daniel Rosenwasser (DanielRosenwasser)

I think you could use a few more edge-case tests. Tests where you're at the beginning of the file, end of the file, and where the code is not syntactically valid (e.g. a closing tag missing its tag name, or a closing fragment tag that "accidentally" has a tag name.

Daniel Rosenwasser (DanielRosenwasser) · 2023-03-31T22:34:26Z


 export const enum CommandTypes {
 JsxClosingTag = "jsxClosingTag",
+ LinkedEditing = "LinkedEditing",


I'm guessing LSP uses this name, but every other command type in the TS Server protocol is mostly camelCase.

Daniel Rosenwasser (DanielRosenwasser) · 2023-03-31T22:38:14Z

+ interface LinkedEditingRequest extends FileLocationRequest {
+ readonly command: CommandTypes.LinkedEditing;
+ }
+ interface LinkedEditingRanges {


It's odd that the ranges are a subfield. Looking at existing precedent, there's FileReferencesResponseBody, so maybe:
Suggested change
interfaceLinkedEditingRanges{
interfaceLinkedEditingRangesBody{

I had written the LinkedEditingRanges and LinkedEditingRangeResponse this way, as this is how they were set up in the stubs and LSP

I would also prefer if it matched the style we had in tsserver and was named Body; LSP may define it one way but I don't think we should be inconsistent to match them.

Matt Bierner (mjbvz) · 2023-04-03T18:16:30Z

+
+ return {
+ ranges: [{ start: openTagStart, length: openTagEnd - openTagStart }, { start: closeTagStart, length: closeTagEnd - closeTagStart }],
+ wordPattern: openingTagText


This should actually be regex for valid tag names. Something like [a-z\._$][a-z0-9\._$]+ perhaps. You can also try omitting it if VS Code's word pattern is good
With the current implementation, VS Code stops linked editing as soon you type anything that is not the tag name

What happens if the returned ranges don't satisfy the regex? No linked cursors I assume?
I think the problem with that regex is that it expects at least two characters. Maybe you wanted:
[a-z\._$][a-z0-9\._$]*
?

Yes, but also try omitting it and seeing if VS Code does the right thing already. You should only need to return wordRange if the default behavior isn't correct

Yeah, in that case let's leave off wordPattern for now.

TypeScript Bot (typescript-bot) · 2023-04-06T22:20:58Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Ryan Cavanaugh (RyanCavanaugh) · 2023-04-06T23:12:15Z

 }

+ type LinkedEditingInfo = {
+ readonly ranges : { start: number, length: number }[];


Suggested change
readonly ranges: {start: number,length: number}[];
readonly ranges: {start: number,length: number}[];

Ryan Cavanaugh (RyanCavanaugh) · 2023-04-06T23:12:27Z


+ type LinkedEditingInfo = {
+ readonly ranges : { start: number, length: number }[];
+ wordPattern? : string;


Suggested change
wordPattern?: string;
wordPattern?: string;

Ryan Cavanaugh (RyanCavanaugh) · 2023-04-06T23:26:08Z

+ }
+ return false;
+ });
+ if (!element || !(isJsxOpeningElement(element) || isJsxClosingElement(element))) return undefined;


Is (isJsxOpeningElement(element) || isJsxClosingElement(element)) possibly false given what the findAncestor call does? Fine to leave in just in case, but I'm curious if this/how this happens

Seems to now be mostly for the typeguard feature, as it seems other code I've added since covers these cases. Should I change it to an as JsxOpeningElement | JsxClosingElement | undefined assertion to the result of findAncestor?

Jake Bailey (jakebailey)

Things seem good, but, I think I would like to see the protocol naming be consistent, as once we add it, we're not going to change it.

Jake Bailey (jakebailey) · 2023-04-06T23:35:10Z

+ interface LinkedEditingRequest extends FileLocationRequest {
+ readonly command: CommandTypes.LinkedEditing;
+ }
+ interface LinkedEditingRanges {


I would also prefer if it matched the style we had in tsserver and was named Body; LSP may define it one way but I don't think we should be inconsistent to match them.

Jake Bailey (jakebailey) · 2023-04-06T23:36:35Z

+verify.linkedEditing( {
+ "0": linkedCursors,
+ "1": linkedCursors,
+ "2": undefined,


Maybe this is a lot of work, but maybe it's better to have a baseline for this instead, rather than all of these undefineds? I know that a lot of our current tests have this, but, I think we're trying to make them friendlier, e.g. Nathan Shively-Sanders (@sandersn)'s new completion baselines.

Jake Bailey (jakebailey) · 2023-04-06T23:37:25Z

+ // determines if the cursor is in an element tag
+ const element = findAncestor(token.parent,
+ n => {
+ if (!n.parent) return "quit";


Is this required? I think the only case where you won't have a parent is on SourceFile, in which case iteration is going to stop.

It caused an issue when you checked the kind of .parent, and .parent is undefined, but turns out, I was able to rewrite findAncestor so I didn't need to check .parent anymore

TypeScript Bot (typescript-bot) · 2023-04-07T19:31:09Z

The TypeScript team hasn't accepted the linked issue #51832. If you can get it accepted, this PR will have a better chance of being reviewed.

Isabel Duan (iisaduan) added 11 commits March 8, 2023 17:38

add tests and fourslash framework
409d949

cd
94c4b65

first implementation, all tests except 3 (namespace) passes
a5192e8

forgot to include into last commit
21aa9e7

passes tests!
398f673

change testing structure
2bafe8d

change testing structure
da0ddf1

add correct content into tests (and some more test cases)
9d7f32c

changes to getLinkedEdit.., saving some extra code in comments as well
1fff99d

remove comments, extra code, rename functions
0b0541d

remove commented code and todos, also renames functions
b70ee90

TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 16, 2023

TypeScript Bot (typescript-bot) added the lib update PR modifies files in the `lib` folder label Mar 16, 2023

Isabel Duan (iisaduan) added 2 commits March 16, 2023 00:13

revert changes to lib
25393f1

Merge branch 'mirror' ofhttps://github.com/iisaduan/TypeScriptinto …
bb98df0
…mirror

Matt Bierner (mjbvz) suggested changes Mar 16, 2023
View reviewed changes

Comment threadtests/baselines/reference/api/typescript.d.ts Outdated
Comment threadtests/baselines/reference/api/tsserverlibrary.d.ts Outdated

Isabel Duan (iisaduan) added 5 commits March 16, 2023 13:19

link everything together (fix session client)
6ba6321

missed renames
92875ce

linted
57ef9b8

update baselines
06afbbd

debugged whitespace issues
f4ec7bd

Isabel Duan (iisaduan) changed the title ~~Adds linked editing for JSX tags, as specified in #51832~~Adds linked editing for JSX tagsMar 17, 2023

Daniel Rosenwasser (DanielRosenwasser) requested changes Mar 17, 2023
View reviewed changes

Isabel Duan (iisaduan) added 5 commits March 20, 2023 14:11

fixed some whitespace in code
803b292

adds better behavior for whitespace and trivia in fragments
34f5e40

refactor getJsxLinkedEditAtPosition
03fa5bb

change type to TextSpan
4faee99

updated tests to have textspan
6a1d3dd

Isabel Duan (iisaduan) added 3 commits March 31, 2023 14:08

cleaned up tests
7f2f885

rename to match lsp
5164495

rename tests
a3bdf2e

Isabel Duan (iisaduan) requested review from Daniel Rosenwasser (DanielRosenwasser) and Matt Bierner (mjbvz)March 31, 2023 22:04

Daniel Rosenwasser (DanielRosenwasser) requested changes Mar 31, 2023
View reviewed changes

more-readable code/comments in getLinkedEditingAtPosition
228e0ae

Matt Bierner (mjbvz) reviewed Apr 3, 2023
View reviewed changes

Daniel Rosenwasser (DanielRosenwasser) reviewed Apr 3, 2023
View reviewed changes

Comment threadsrc/server/session.ts Outdated

Isabel Duan (iisaduan) added 2 commits April 4, 2023 15:29

add more tests + add range to function names
32e43c8

more tests
da4f1cd

Isabel Duan (iisaduan) marked this pull request as ready for review April 6, 2023 22:20

Ryan Cavanaugh (RyanCavanaugh) reviewed Apr 6, 2023
View reviewed changes

Ryan Cavanaugh (RyanCavanaugh) approved these changes Apr 6, 2023
View reviewed changes

Jake Bailey (jakebailey) reviewed Apr 6, 2023
View reviewed changes

update Body, simplify call to findAncestor
db67aee

Jake Bailey (jakebailey) approved these changes Apr 7, 2023
View reviewed changes

TypeScript Bot (typescript-bot) assigned Isabel Duan (iisaduan)Apr 7, 2023

TypeScript Bot (typescript-bot) added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 7, 2023

Isabel Duan (iisaduan) merged commit d4c48e1 into microsoft:mainApr 7, 2023

qiqiboy (qiqiboy) mentioned this pull request Apr 10, 2023
[feature request]Add linkedEditing LSP feature neoclide/coc-tsserver#423
Closed

Sebastian "Sebbie" Silbermann (eps1lon) mentioned this pull request Apr 15, 2023
[test] Intrinsics with colon no longer type-checked #53784
Closed

Isabel Duan (iisaduan) mentioned this pull request Apr 20, 2023
Linked editing disconnects on valid tag names with non alphanumeric characters #53936
Closed

Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025

	interfaceLinkedEditingRanges{
	interfaceLinkedEditingRangesBody{

	readonly ranges: {start: number,length: number}[];
	readonly ranges: {start: number,length: number}[];

Uh oh!

Conversation

Isabel Duan (iisaduan) commented Mar 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 16, 2023

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 16, 2023

Uh oh!

Uh oh!

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Isabel Duan (iisaduan) commented Mar 31, 2023

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TypeScript Bot (typescript-bot) commented Apr 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jake Bailey (jakebailey) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TypeScript Bot (typescript-bot) commented Apr 7, 2023

Uh oh!

Reviewers

Isabel Duan (iisaduan) commented Mar 16, 2023•
edited
Loading

Jake Bailey (jakebailey) left a comment •
edited
Loading