Skip to content

Commit ef87953

Browse files
atscottzarend
atscott
authored and
zarend
committed
fix(language-service): only provide template results on reference requests (#41041)
VSCode only de-duplicates references results for "go to references" requests but does not de-duplicate them for "find all references" requests. The result is that users see duplicate references for results in TypeScript files - one from the built-in TS extension and one from us. While this is an issue in VSCode (see microsoft/vscode#117095) this commit provides a quick workaround on our end until it can be addressed there. This commit should be reverted when microsoft/vscode/issues/117095 is resolved. fixes angular/vscode-ng-language-service#1124 PR Close #41041
1 parent ac268d1 commit ef87953

File tree

2 files changed

+61
-62
lines changed

2 files changed

+61
-62
lines changed

‎packages/language-service/ivy/references.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,9 @@ export class ReferencesAndRenameBuilder {
352352
entries.set(createLocationKey(entry), entry);
353353
}
354354
} else {
355-
entries.set(createLocationKey(ref), ref);
355+
// TODO(atscott): uncomment when VSCode deduplicates results on their end
356+
// https://github.com/microsoft/vscode/issues/117095
357+
// entries.set(createLocationKey(ref), ref);
356358
}
357359
}
358360
return Array.from(entries.values());

‎packages/language-service/ivy/test/references_spec.ts‎

Lines changed: 58 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ describe('find references and rename locations', () => {
4343

4444
it('gets component member references from TS file and external template', () => {
4545
const refs = getReferencesAtPosition(appFile)!;
46-
expect(refs.length).toBe(2);
47-
assertFileNames(refs, ['app.html', 'app.ts']);
46+
expect(refs.length).toBe(1);
47+
assertFileNames(refs, ['app.html']);
4848
assertTextSpans(refs, ['myProp']);
4949
});
5050

@@ -78,8 +78,8 @@ describe('find references and rename locations', () => {
7878

7979
it('gets references', () => {
8080
const refs = getReferencesAtPosition(templateFile)!;
81-
expect(refs.length).toBe(2);
82-
assertFileNames(refs, ['app.html', 'app.ts']);
81+
expect(refs.length).toBe(1);
82+
assertFileNames(refs, ['app.html']);
8383
assertTextSpans(refs, ['myProp']);
8484
});
8585

@@ -112,7 +112,7 @@ describe('find references and rename locations', () => {
112112

113113
it('gets component member reference in ts file', () => {
114114
const refs = getReferencesAtPosition(appFile)!;
115-
expect(refs.length).toBe(2);
115+
expect(refs.length).toBe(1);
116116

117117
assertFileNames(refs, ['app.ts']);
118118
assertTextSpans(refs, ['setTitle']);
@@ -149,7 +149,7 @@ describe('find references and rename locations', () => {
149149

150150
it('gets member reference in ts file', () => {
151151
const refs = getReferencesAtPosition(appFile)!;
152-
expect(refs.length).toBe(2);
152+
expect(refs.length).toBe(1);
153153

154154
assertTextSpans(refs, ['title']);
155155
});
@@ -215,9 +215,9 @@ describe('find references and rename locations', () => {
215215

216216
it('gets member reference in ts file', () => {
217217
const refs = getReferencesAtPosition(file)!;
218-
expect(refs.length).toBe(2);
218+
expect(refs.length).toBe(1);
219219

220-
assertFileNames(refs, ['app.ts', 'app.html']);
220+
assertFileNames(refs, ['app.html']);
221221
assertTextSpans(refs, ['title']);
222222
});
223223

@@ -253,7 +253,7 @@ describe('find references and rename locations', () => {
253253

254254
it('get reference to member in ts file', () => {
255255
const refs = getReferencesAtPosition(file)!;
256-
expect(refs.length).toBe(2);
256+
expect(refs.length).toBe(1);
257257

258258
assertFileNames(refs, ['app.ts']);
259259
assertTextSpans(refs, ['otherTitle']);
@@ -290,15 +290,15 @@ describe('find references and rename locations', () => {
290290
it('gets reference to member type definition and initialization in component class', () => {
291291
const refs = getReferencesAtPosition(file)!;
292292
// 3 references: the type definition, the value assignment, and the read in the template
293-
expect(refs.length).toBe(3);
293+
expect(refs.length).toBe(1);
294294

295295
assertFileNames(refs, ['app.ts']);
296296
// TODO(atscott): investigate if we can make the template keyed read be just the 'name' part.
297297
// The TypeScript implementation specifically adjusts the span to accommodate string literals:
298298
// https://sourcegraph.com/github.com/microsoft/TypeScript@d5779c75d3dd19565b60b9e2960b8aac36d4d635/-/blob/src/services/findAllReferences.ts#L508-512
299299
// One possible solution would be to extend `FullTemplateMapping` to include the matched TCB
300300
// node and then do the same thing that TS does: if the node is a string, adjust the span.
301-
assertTextSpans(refs, ['name', '"name"']);
301+
assertTextSpans(refs, ['"name"']);
302302
});
303303

304304
it('gets rename locations in component class', () => {
@@ -338,9 +338,9 @@ describe('find references and rename locations', () => {
338338

339339
it('get references in ts file', () => {
340340
const refs = getReferencesAtPosition(file)!;
341-
expect(refs.length).toBe(2);
341+
expect(refs.length).toBe(1);
342342

343-
assertFileNames(refs, ['app.ts', 'app.html']);
343+
assertFileNames(refs, ['app.html']);
344344
assertTextSpans(refs, ['batman']);
345345
});
346346

@@ -493,8 +493,8 @@ describe('find references and rename locations', () => {
493493

494494
it('should get references', () => {
495495
const refs = getReferencesAtPosition(file)!;
496-
expect(refs.length).toBe(2);
497-
assertFileNames(refs, ['dir.ts', 'app.html']);
496+
expect(refs.length).toBe(1);
497+
assertFileNames(refs, ['app.html']);
498498
assertTextSpans(refs, ['dirValue']);
499499
});
500500

@@ -523,8 +523,8 @@ describe('find references and rename locations', () => {
523523

524524
it('should get references', () => {
525525
const refs = getReferencesAtPosition(file)!;
526-
expect(refs.length).toBe(2);
527-
assertFileNames(refs, ['dir.ts', 'app.html']);
526+
expect(refs.length).toBe(1);
527+
assertFileNames(refs, ['app.html']);
528528
assertTextSpans(refs, ['dirValue']);
529529
});
530530

@@ -553,8 +553,8 @@ describe('find references and rename locations', () => {
553553

554554
it('should get references', () => {
555555
const refs = getReferencesAtPosition(file)!;
556-
expect(refs.length).toBe(2);
557-
assertFileNames(refs, ['dir.ts', 'app.html']);
556+
expect(refs.length).toBe(1);
557+
assertFileNames(refs, ['app.html']);
558558
assertTextSpans(refs, ['doSomething']);
559559
});
560560

@@ -693,8 +693,8 @@ describe('find references and rename locations', () => {
693693

694694
it('should find references', () => {
695695
const refs = getReferencesAtPosition(file)!;
696-
expect(refs.length).toBe(2);
697-
assertFileNames(refs, ['app.ts', 'example-directive.ts']);
696+
expect(refs.length).toBe(1);
697+
assertFileNames(refs, ['app.ts']);
698698
assertTextSpans(refs, ['identifier']);
699699
});
700700

@@ -727,7 +727,7 @@ describe('find references and rename locations', () => {
727727

728728
it('should find references', () => {
729729
const refs = getReferencesAtPosition(file)!;
730-
expect(refs.length).toBe(2);
730+
expect(refs.length).toBe(1);
731731
assertFileNames(refs, ['app.ts']);
732732
assertTextSpans(refs, ['name']);
733733
});
@@ -777,9 +777,9 @@ describe('find references and rename locations', () => {
777777

778778
it('should find references', () => {
779779
const refs = getReferencesAtPosition(file)!;
780-
expect(refs.length).toBe(5);
781-
assertFileNames(refs, ['index.d.ts', 'prefix-pipe.ts', 'app.ts']);
782-
assertTextSpans(refs, ['transform', 'prefixPipe']);
780+
expect(refs.length).toBe(1);
781+
assertFileNames(refs, ['app.ts']);
782+
assertTextSpans(refs, ['prefixPipe']);
783783
});
784784

785785
it('should find rename locations', () => {
@@ -817,7 +817,7 @@ describe('find references and rename locations', () => {
817817

818818
it('should find references', () => {
819819
const refs = getReferencesAtPosition(file)!;
820-
expect(refs.length).toBe(2);
820+
expect(refs.length).toBe(1);
821821
assertFileNames(refs, ['app.ts']);
822822
assertTextSpans(refs, ['prefix']);
823823
});
@@ -862,8 +862,8 @@ describe('find references and rename locations', () => {
862862

863863
it('should find references', () => {
864864
const refs = getReferencesAtPosition(file)!;
865-
expect(refs.length).toEqual(2);
866-
assertFileNames(refs, ['string-model.ts', 'app.ts']);
865+
expect(refs.length).toEqual(1);
866+
assertFileNames(refs, ['app.ts']);
867867
assertTextSpans(refs, ['model']);
868868
});
869869

@@ -947,8 +947,8 @@ describe('find references and rename locations', () => {
947947

948948
it('should work for text attributes', () => {
949949
const refs = getReferencesAtPosition(file)!;
950-
expect(refs.length).toEqual(2);
951-
assertFileNames(refs, ['string-model.ts', 'app.ts']);
950+
expect(refs.length).toEqual(1);
951+
assertFileNames(refs, ['app.ts']);
952952
assertTextSpans(refs, ['model']);
953953
});
954954

@@ -988,8 +988,8 @@ describe('find references and rename locations', () => {
988988

989989
it('should work from the TS input declaration', () => {
990990
const refs = getReferencesAtPosition(file)!;
991-
expect(refs.length).toEqual(2);
992-
assertFileNames(refs, ['app.ts', 'string-model.ts']);
991+
expect(refs.length).toEqual(1);
992+
assertFileNames(refs, ['app.ts']);
993993
assertTextSpans(refs, ['model']);
994994
});
995995

@@ -1041,8 +1041,8 @@ describe('find references and rename locations', () => {
10411041

10421042
it('should find references', () => {
10431043
const refs = getReferencesAtPosition(file)!;
1044-
expect(refs.length).toEqual(3);
1045-
assertFileNames(refs, ['app.ts', 'string-model.ts', 'other-dir.ts']);
1044+
expect(refs.length).toEqual(1);
1045+
assertFileNames(refs, ['app.ts']);
10461046
assertTextSpans(refs, ['model']);
10471047
});
10481048

@@ -1076,9 +1076,9 @@ describe('find references and rename locations', () => {
10761076

10771077
it('should find references', () => {
10781078
const refs = getReferencesAtPosition(file)!;
1079-
expect(refs.length).toEqual(2);
1080-
assertFileNames(refs, ['string-model.ts', 'app.ts']);
1081-
assertTextSpans(refs, ['aliasedModel', 'alias']);
1079+
expect(refs.length).toEqual(1);
1080+
assertFileNames(refs, ['app.ts']);
1081+
assertTextSpans(refs, ['alias']);
10821082
});
10831083

10841084
it('should find rename locations', () => {
@@ -1130,7 +1130,7 @@ describe('find references and rename locations', () => {
11301130

11311131
it('should find references', () => {
11321132
const refs = getReferencesAtPosition(file)!;
1133-
expect(refs.length).toEqual(2);
1133+
expect(refs.length).toEqual(1);
11341134
assertTextSpans(refs, ['modelChange']);
11351135
});
11361136

@@ -1154,8 +1154,8 @@ describe('find references and rename locations', () => {
11541154

11551155
it('should find references', () => {
11561156
const refs = getReferencesAtPosition(file)!;
1157-
expect(refs.length).toEqual(2);
1158-
assertTextSpans(refs, ['aliasedModelChange', 'alias']);
1157+
expect(refs.length).toEqual(1);
1158+
assertTextSpans(refs, ['alias']);
11591159
});
11601160

11611161
it('should find rename locations', () => {
@@ -1194,12 +1194,9 @@ describe('find references and rename locations', () => {
11941194
file.moveCursorToText('[(mod¦el)]');
11951195

11961196
const refs = getReferencesAtPosition(file)!;
1197-
// Note that this includes the 'model` twice from the template. As with other potential
1198-
// duplicates (like if another plugin returns the same span), we expect the LS clients to filter
1199-
// these out themselves.
1200-
expect(refs.length).toEqual(4);
1201-
assertFileNames(refs, ['dir.ts', 'app.ts']);
1202-
assertTextSpans(refs, ['model', 'modelChange']);
1197+
expect(refs.length).toEqual(2);
1198+
assertFileNames(refs, ['app.ts']);
1199+
assertTextSpans(refs, ['model']);
12031200
});
12041201

12051202
describe('directives', () => {
@@ -1235,9 +1232,9 @@ describe('find references and rename locations', () => {
12351232
const refs = getReferencesAtPosition(file)!;
12361233
// 4 references are: class declaration, template usage, app import and use in declarations
12371234
// list.
1238-
expect(refs.length).toBe(4);
1239-
assertTextSpans(refs, ['<div dir>', 'Dir']);
1240-
assertFileNames(refs, ['app.ts', 'dir.ts']);
1235+
expect(refs.length).toBe(1);
1236+
assertTextSpans(refs, ['<div dir>']);
1237+
assertFileNames(refs, ['app.ts']);
12411238
});
12421239

12431240
it('should find rename locations', () => {
@@ -1284,9 +1281,9 @@ describe('find references and rename locations', () => {
12841281

12851282
it('gets references to all matching directives', () => {
12861283
const refs = getReferencesAtPosition(file)!;
1287-
expect(refs.length).toBe(8);
1288-
assertTextSpans(refs, ['<div dir>', 'Dir', 'Dir2']);
1289-
assertFileNames(refs, ['app.ts', 'dir.ts', 'dir2.ts']);
1284+
expect(refs.length).toBe(2);
1285+
assertTextSpans(refs, ['<div dir>']);
1286+
assertFileNames(refs, ['app.ts']);
12901287
});
12911288

12921289
it('finds rename locations for all matching directives', () => {
@@ -1321,9 +1318,9 @@ describe('find references and rename locations', () => {
13211318

13221319
it('should be able to request references', () => {
13231320
const refs = getReferencesAtPosition(file)!;
1324-
expect(refs.length).toBe(6);
1325-
assertTextSpans(refs, ['<div *ngFor="let item of items"></div>', 'NgForOf']);
1326-
assertFileNames(refs, ['index.d.ts', 'app.ts']);
1321+
expect(refs.length).toBe(1);
1322+
assertTextSpans(refs, ['<div *ngFor="let item of items"></div>']);
1323+
assertFileNames(refs, ['app.ts']);
13271324
});
13281325

13291326
it('should not support rename if directive is in a dts file', () => {
@@ -1363,9 +1360,9 @@ describe('find references and rename locations', () => {
13631360
const refs = getReferencesAtPosition(file)!;
13641361
// 4 references are: class declaration, template usage, app import and use in declarations
13651362
// list.
1366-
expect(refs.length).toBe(4);
1367-
assertTextSpans(refs, ['<my-comp>', 'MyComp']);
1368-
assertFileNames(refs, ['app.ts', 'comp.ts']);
1363+
expect(refs.length).toBe(1);
1364+
assertTextSpans(refs, ['<my-comp>']);
1365+
assertFileNames(refs, ['app.ts']);
13691366
});
13701367

13711368
it('gets rename locations', () => {
@@ -1408,9 +1405,9 @@ describe('find references and rename locations', () => {
14081405
const refs = getReferencesAtPosition(file)!;
14091406
// 4 references are: class declaration, template usage, app import and use in declarations
14101407
// list.
1411-
expect(refs.length).toBe(4);
1412-
assertTextSpans(refs, ['<my-comp>', 'MyComp']);
1413-
assertFileNames(refs, ['app.ts', 'comp.ts']);
1408+
expect(refs.length).toBe(1);
1409+
assertTextSpans(refs, ['<my-comp>']);
1410+
assertFileNames(refs, ['app.ts']);
14141411
});
14151412

14161413
it('finds rename locations', () => {

0 commit comments

Comments
 (0)