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

Adds linked editing for JSX tags #53284

Merged
merged 34 commits into from
Apr 7, 2023
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
409d949
add tests and fourslash framework
iisaduan Mar 9, 2023
94c4b65
cd
iisaduan Mar 11, 2023
a5192e8
first implementation, all tests except 3 (namespace) passes
iisaduan Mar 11, 2023
21aa9e7
forgot to include into last commit
iisaduan Mar 11, 2023
398f673
passes tests!
iisaduan Mar 13, 2023
2bafe8d
change testing structure
iisaduan Mar 15, 2023
da0ddf1
change testing structure
iisaduan Mar 16, 2023
9d7f32c
add correct content into tests (and some more test cases)
iisaduan Mar 16, 2023
1fff99d
changes to getLinkedEdit.., saving some extra code in comments as well
iisaduan Mar 16, 2023
0b0541d
remove comments, extra code, rename functions
iisaduan Mar 16, 2023
b70ee90
remove commented code and todos, also renames functions
iisaduan Mar 16, 2023
25393f1
revert changes to lib
iisaduan Mar 16, 2023
bb98df0
Merge branch 'mirror' of https://github.com/iisaduan/TypeScript into …
iisaduan Mar 16, 2023
6ba6321
link everything together (fix session client)
iisaduan Mar 16, 2023
92875ce
missed renames
iisaduan Mar 16, 2023
57ef9b8
linted
iisaduan Mar 16, 2023
06afbbd
update baselines
iisaduan Mar 16, 2023
f4ec7bd
debugged whitespace issues
iisaduan Mar 17, 2023
803b292
fixed some whitespace in code
iisaduan Mar 20, 2023
34f5e40
adds better behavior for whitespace and trivia in fragments
iisaduan Mar 22, 2023
03fa5bb
refactor getJsxLinkedEditAtPosition
iisaduan Mar 22, 2023
4faee99
change type to TextSpan
iisaduan Mar 22, 2023
6a1d3dd
updated tests to have textspan
iisaduan Mar 23, 2023
c8ea559
fix names
iisaduan Mar 27, 2023
fef7ab6
fix names
iisaduan Mar 27, 2023
bb720e1
Merge branch 'mirror' of https://github.com/iisaduan/TypeScript into …
iisaduan Mar 28, 2023
be34a1e
update protocol, refactor services
iisaduan Mar 31, 2023
7f2f885
cleaned up tests
iisaduan Mar 31, 2023
5164495
rename to match lsp
iisaduan Mar 31, 2023
a3bdf2e
rename tests
iisaduan Mar 31, 2023
228e0ae
more-readable code/comments in getLinkedEditingAtPosition
iisaduan Apr 3, 2023
32e43c8
add more tests + add `range` to function names
iisaduan Apr 4, 2023
da4f1cd
more tests
iisaduan Apr 6, 2023
db67aee
update `Body`, simplify call to `findAncestor`
iisaduan Apr 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/harness/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,10 @@ export class SessionClient implements LanguageService {
return notImplemented();
}

getLinkedEditingAtPosition(_fileName: string, _position: number): never {
return notImplemented();
}

getSpanOfEnclosingComment(_fileName: string, _position: number, _onlyMultiLine: boolean): TextSpan {
return notImplemented();
}
Expand Down
8 changes: 8 additions & 0 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3444,6 +3444,14 @@ export class TestState {
}
}

public verifyLinkedEditing(map: { [markerName: string]: ts.LinkedEditingInfo | undefined }): void {
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
for (const markerName in map) {
this.goToMarker(markerName);
const actual = this.languageService.getLinkedEditingAtPosition(this.activeFile.fileName, this.currentCaretPosition);
assert.deepEqual(actual, map[markerName], markerName);
}
}

public verifyMatchingBracePosition(bracePosition: number, expectedMatchPosition: number) {
const actual = this.languageService.getBraceMatchingAtPosition(this.activeFile.fileName, bracePosition);

Expand Down
4 changes: 4 additions & 0 deletions src/harness/fourslashInterfaceImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,10 @@ export class VerifyNegatable {
this.state.verifyJsxClosingTag(map);
}

public linkedEditing(map: { [markerName: string]: ts.LinkedEditingInfo | undefined }): void {
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
this.state.verifyLinkedEditing(map);
}

iisaduan marked this conversation as resolved.
Show resolved Hide resolved
public isInCommentAtPosition(onlyMultiLineDiverges?: boolean) {
this.state.verifySpanOfEnclosingComment(this.negative, onlyMultiLineDiverges);
}
Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,9 @@ class LanguageServiceShimProxy implements ts.LanguageService {
getJsxClosingTagAtPosition(): never {
throw new Error("Not supported on the shim.");
}
getLinkedEditingAtPosition(): never {
throw new Error("Not supported on the shim.");
}
getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): ts.TextSpan {
return unwrapJSONCallResult(this.shim.getSpanOfEnclosingComment(fileName, position, onlyMultiLine));
}
Expand Down
13 changes: 13 additions & 0 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {

export const enum CommandTypes {
JsxClosingTag = "jsxClosingTag",
LinkedEditing = "LinkedEditing",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Brace = "brace",
/** @internal */
BraceFull = "brace-full",
Expand Down Expand Up @@ -1101,6 +1102,18 @@ export interface JsxClosingTagResponse extends Response {
readonly body: TextInsertion;
}

export interface LinkedEditingRequest extends FileLocationRequest {
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
readonly command: CommandTypes.LinkedEditing;
}

export interface LinkedEditingRanges {
ranges: TextSpan[];
wordPattern?: string;
}

iisaduan marked this conversation as resolved.
Show resolved Hide resolved
export interface LinkedEditingResponse extends Response {
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
readonly body: LinkedEditingRanges;
}

/**
* Get document highlights request; value of command field is
Expand Down
27 changes: 27 additions & 0 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import {
JSDocTagInfo,
LanguageServiceMode,
LineAndCharacter,
LinkedEditingInfo,
map,
mapDefined,
mapDefinedIterator,
Expand Down Expand Up @@ -1802,6 +1803,15 @@ export class Session<TMessage = string> implements EventSender {
return tag === undefined ? undefined : { newText: tag.newText, caretOffset: 0 };
}

private getLinkedEditing(args: protocol.FileLocationRequestArgs): protocol.LinkedEditingRanges | undefined {
const { file, languageService } = this.getFileAndLanguageServiceForSyntacticOperation(args);
const position = this.getPositionInFile(args, file);
const linkedEditInfo = languageService.getLinkedEditingAtPosition(file, position);
const scriptInfo = this.projectService.getScriptInfoForNormalizedPath(file);
if (scriptInfo === undefined || linkedEditInfo === undefined) return undefined;
return convertLinkedEditInfoToRanges(linkedEditInfo, scriptInfo);
}

private getDocumentHighlights(args: protocol.DocumentHighlightsRequestArgs, simplifiedResult: boolean): readonly protocol.DocumentHighlightsItem[] | readonly DocumentHighlights[] {
const { file, project } = this.getFileAndProject(args);
const position = this.getPositionInFile(args, file);
Expand Down Expand Up @@ -3389,6 +3399,9 @@ export class Session<TMessage = string> implements EventSender {
[protocol.CommandTypes.JsxClosingTag]: (request: protocol.JsxClosingTagRequest) => {
return this.requiredResponse(this.getJsxClosingTag(request.arguments));
},
[protocol.CommandTypes.LinkedEditing]: (request: protocol.LinkedEditingRequest) => {
return this.requiredResponse(this.getLinkedEditing(request.arguments));
},
[protocol.CommandTypes.GetCodeFixes]: (request: protocol.CodeFixRequest) => {
return this.requiredResponse(this.getCodeFixes(request.arguments, /*simplifiedResult*/ true));
},
Expand Down Expand Up @@ -3644,6 +3657,20 @@ function positionToLineOffset(info: ScriptInfoOrConfig, position: number): proto
return isConfigFile(info) ? locationFromLineAndCharacter(info.getLineAndCharacterOfPosition(position)) : info.positionToLineOffset(position);
}

function convertLinkedEditInfoToRanges(linkedEdit: LinkedEditingInfo, scriptInfo: ScriptInfo): protocol.LinkedEditingRanges {
return {
ranges: linkedEdit.ranges.map(
s => {
return {
start: scriptInfo.positionToLineOffset(s.start),
end: scriptInfo.positionToLineOffset(s.start + s.length)
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
};
}
),
wordPattern: linkedEdit.wordPattern,
};
}

function locationFromLineAndCharacter(lc: LineAndCharacter): protocol.Location {
return { line: lc.line + 1, offset: lc.character + 1 };
}
Expand Down
55 changes: 55 additions & 0 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ import {
filter,
find,
FindAllReferences,
findAncestor,
findChildOfKind,
findPrecedingToken,
first,
Expand Down Expand Up @@ -185,10 +186,12 @@ import {
JSDocTagInfo,
JsonSourceFile,
JsxAttributes,
JsxClosingElement,
JsxClosingTagInfo,
JsxElement,
JsxEmit,
JsxFragment,
JsxOpeningElement,
LanguageService,
LanguageServiceHost,
LanguageServiceMode,
Expand All @@ -197,6 +200,7 @@ import {
length,
LineAndCharacter,
lineBreakPart,
LinkedEditingInfo,
LiteralType,
map,
mapDefined,
Expand Down Expand Up @@ -2478,6 +2482,56 @@ export function createLanguageService(
}
}

function getLinkedEditingAtPosition(fileName: string, position: number): LinkedEditingInfo | undefined {
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName);
const token = findPrecedingToken(position, sourceFile);
if (!token) return undefined;

if (isJsxFragment(token.parent.parent)) {
const openPos = token.parent.parent.openingFragment.getStart(sourceFile) + 1; // "<".length
const closePos = token.parent.parent.closingFragment.getStart(sourceFile) + 2; // "</".length

// only allows linked editing right after opening bracket: <| ></| >
if ((position !== openPos) && (position !== closePos)) return undefined;

// wordPattern is undefined since fragments have no tag name
return { ranges: [{ start: openPos, length: 0 }, { start: closePos, length: 0 }] };
}
else {
// determines if the cursor is in an element tag
const tag = findAncestor(token.parent,
n => {
if (!n.parent) return "quit";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else if (isJsxElement(n.parent)) {
if (isJsxOpeningElement(n) || isJsxClosingElement(n)) {
return true;
}
return "quit";
}
return false;
});
if (!tag) return undefined;

const element = tag as JsxOpeningElement | JsxClosingElement;
const openTagStart = element.parent.openingElement.tagName.getStart(sourceFile);
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
const openTagEnd = element.parent.openingElement.tagName.end;
const closeTagStart = element.parent.closingElement.tagName.getStart(sourceFile);
const closeTagEnd = element.parent.closingElement.tagName.end;

// only return linked cursors if the cursor is within a tag name
if (!(openTagStart <= position && position <= openTagEnd || closeTagStart <= position && position <= closeTagEnd)) return undefined;

// only return linked cursors if text in both tags is identical
const openingTagText = element.parent.openingElement.tagName.getText(sourceFile);
if (openingTagText !== element.parent.closingElement.tagName.getText(sourceFile)) return undefined;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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\._$]*

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
}
}

function getLinesForRange(sourceFile: SourceFile, textRange: TextRange) {
return {
lineStarts: sourceFile.getLineStarts(),
Expand Down Expand Up @@ -3009,6 +3063,7 @@ export function createLanguageService(
getDocCommentTemplateAtPosition,
isValidBraceCompletionAtPosition,
getJsxClosingTagAtPosition,
getLinkedEditingAtPosition,
getSpanOfEnclosingComment,
getCodeFixesAtPosition,
getCombinedCodeFix,
Expand Down
6 changes: 6 additions & 0 deletions src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,7 @@ export interface LanguageService {
* Editors should call this after `>` is typed.
*/
getJsxClosingTagAtPosition(fileName: string, position: number): JsxClosingTagInfo | undefined;
getLinkedEditingAtPosition(fileName: string, position: number): LinkedEditingInfo | undefined;

getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): TextSpan | undefined;

Expand Down Expand Up @@ -661,6 +662,11 @@ export interface JsxClosingTagInfo {
readonly newText: string;
}

export interface LinkedEditingInfo {
readonly ranges: TextSpan[];
wordPattern?: string;
}

export interface CombinedCodeFixScope { type: "file"; fileName: string; }

export const enum OrganizeImportsMode {
Expand Down
17 changes: 17 additions & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ declare namespace ts {
namespace protocol {
enum CommandTypes {
JsxClosingTag = "jsxClosingTag",
LinkedEditing = "LinkedEditing",
Brace = "brace",
BraceCompletion = "braceCompletion",
GetSpanOfEnclosingComment = "getSpanOfEnclosingComment",
Expand Down Expand Up @@ -877,6 +878,16 @@ declare namespace ts {
interface JsxClosingTagResponse extends Response {
readonly body: TextInsertion;
}
interface LinkedEditingRequest extends FileLocationRequest {
readonly command: CommandTypes.LinkedEditing;
}
interface LinkedEditingRanges {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
interface LinkedEditingRanges {
interface LinkedEditingRangesBody {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ranges: TextSpan[];
wordPattern?: string;
}
interface LinkedEditingResponse extends Response {
readonly body: LinkedEditingRanges;
}
/**
* Get document highlights request; value of command field is
* "documentHighlights". Return response giving spans that are relevant
Expand Down Expand Up @@ -3847,6 +3858,7 @@ declare namespace ts {
private getSemanticDiagnosticsSync;
private getSuggestionDiagnosticsSync;
private getJsxClosingTag;
private getLinkedEditing;
private getDocumentHighlights;
private provideInlayHints;
private setCompilerOptionsForInferredProjects;
Expand Down Expand Up @@ -9977,6 +9989,7 @@ declare namespace ts {
* Editors should call this after `>` is typed.
*/
getJsxClosingTagAtPosition(fileName: string, position: number): JsxClosingTagInfo | undefined;
getLinkedEditingAtPosition(fileName: string, position: number): LinkedEditingInfo | undefined;
getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): TextSpan | undefined;
toLineColumnOffset?(fileName: string, position: number): LineAndCharacter;
getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: readonly number[], formatOptions: FormatCodeSettings, preferences: UserPreferences): readonly CodeFixAction[];
Expand Down Expand Up @@ -10006,6 +10019,10 @@ declare namespace ts {
interface JsxClosingTagInfo {
readonly newText: string;
}
interface LinkedEditingInfo {
readonly ranges: TextSpan[];
wordPattern?: string;
}
interface CombinedCodeFixScope {
type: "file";
fileName: string;
Expand Down
5 changes: 5 additions & 0 deletions tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6107,6 +6107,7 @@ declare namespace ts {
* Editors should call this after `>` is typed.
*/
getJsxClosingTagAtPosition(fileName: string, position: number): JsxClosingTagInfo | undefined;
getLinkedEditingAtPosition(fileName: string, position: number): LinkedEditingInfo | undefined;
getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): TextSpan | undefined;
toLineColumnOffset?(fileName: string, position: number): LineAndCharacter;
getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: readonly number[], formatOptions: FormatCodeSettings, preferences: UserPreferences): readonly CodeFixAction[];
Expand Down Expand Up @@ -6136,6 +6137,10 @@ declare namespace ts {
interface JsxClosingTagInfo {
readonly newText: string;
}
interface LinkedEditingInfo {
readonly ranges: TextSpan[];
wordPattern?: string;
}
interface CombinedCodeFixScope {
type: "file";
fileName: string;
Expand Down
3 changes: 3 additions & 0 deletions tests/cases/fourslash/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,9 @@ declare namespace FourSlashInterface {
implementationListIsEmpty(): void;
isValidBraceCompletionAtPosition(openingBrace?: string): void;
jsxClosingTag(map: { [markerName: string]: { readonly newText: string } | undefined }): void;
linkedEditing(map: { [markerName: string]: {
readonly ranges : { start: number, length: number }[],
wordPattern? : string ;} | undefined }): void;
isInCommentAtPosition(onlyMultiLineDiverges?: boolean): void;
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
codeFix(options: {
description: string | [string, ...(string | number)[]] | DiagnosticIgnoredInterpolations,
Expand Down
71 changes: 71 additions & 0 deletions tests/cases/fourslash/linkedEditingJsxTag1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/// <reference path='fourslash.ts' />

// the content of basic.tsx
//const jsx = (
// <div>
// </div>
//);

// @Filename: /basic.tsx
////const jsx = (
//// </*0*/d/*1*/iv/*2*/>/*3*/
//// </*4*///*5*/di/*6*/v/*7*/>/*8*/
////);

// @Filename: /whitespaceInvalidClosing.tsx
////const jsx = (
//// <div>
//// < /*9*/ /div>
////);

// @Filename: /whitespace.tsx
////const whitespaceOpening = (
//// </*10*/ /*11*/div/*12*/ /*13*/> /*14*/
//// <//*15*/di/*16*/v>
////);
////const whitespaceClosing = (
//// </*17*/di/*18*/v>
//// <//*19*/ /*20*/div/*21*/ /*22*/> /*23*/
////);

const markers = test.markers();
const linkedCursors1 = {
ranges: [{ start: markers[0].position, length: 3 }, { start: markers[5].position, length: 3 }],
wordPattern: 'div'
};
const linkedCursors2 = {
ranges: [{ start: markers[11].position, length: 3 }, { start: markers[15].position, length: 3 }],
wordPattern: 'div'
};
const linkedCursors3 = {
ranges: [{ start: markers[17].position, length: 3 }, { start: markers[20].position, length: 3 }],
wordPattern: 'div'
};

verify.linkedEditing( {
"0": linkedCursors1,
"1": linkedCursors1,
"2": linkedCursors1,
"3": undefined,
"4": undefined,
"5": linkedCursors1,
"6": linkedCursors1,
"7": linkedCursors1,
"8": undefined,
"9": undefined, // I believe this should be an invalid tag
"10": undefined,
"11": linkedCursors2,
"12": linkedCursors2,
"13": undefined,
"14": undefined,
"15": linkedCursors2,
"16": linkedCursors2,
"17": linkedCursors3,
"18": linkedCursors3,
"19": undefined,
"20": linkedCursors3,
"21": linkedCursors3,
"22": undefined,
"23": undefined,
});

Loading