Skip to content

Commit

Permalink
fix(language-service): only provide template results on reference req…
Browse files Browse the repository at this point in the history 
…uests

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
  • Loading branch information
@atscott
atscott committed Mar 1, 2021
1 parent 71f2ca0 commit 77e8d03
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 62 deletions.
4 changes: 3 additions & 1 deletion packages/language-service/ivy/references.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,9 @@ export class ReferencesAndRenameBuilder {
entries.set(createLocationKey(entry), entry);
}
} else {
entries.set(createLocationKey(ref), ref);
// TODO(atscott): uncomment when VSCode deduplicates results on their end
// https://github.com/microsoft/vscode/issues/117095
// entries.set(createLocationKey(ref), ref);
}
}
return Array.from(entries.values());
Expand Down
119 changes: 58 additions & 61 deletions packages/language-service/ivy/test/references_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ describe('find references and rename locations', () => {

it('gets component member references from TS file and external template', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
assertFileNames(refs, ['app.html', 'app.ts']);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.html']);
assertTextSpans(refs, ['myProp']);
});

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

it('gets references', () => {
const refs = getReferencesAtPosition(_('/app.html'), cursor)!;
expect(refs.length).toBe(2);
assertFileNames(refs, ['app.html', 'app.ts']);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.html']);
assertTextSpans(refs, ['myProp']);
});

Expand Down Expand Up @@ -109,7 +109,7 @@ describe('find references and rename locations', () => {

it('gets component member reference in ts file', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
expect(refs.length).toBe(1);

assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['setTitle']);
Expand Down Expand Up @@ -143,7 +143,7 @@ describe('find references and rename locations', () => {

it('gets member reference in ts file', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
expect(refs.length).toBe(1);

assertTextSpans(refs, ['title']);
});
Expand Down Expand Up @@ -207,9 +207,9 @@ describe('find references and rename locations', () => {

it('gets member reference in ts file', () => {
const refs = getReferencesAtPosition(_('/app.html'), cursor)!;
expect(refs.length).toBe(2);
expect(refs.length).toBe(1);

assertFileNames(refs, ['app.ts', 'app.html']);
assertFileNames(refs, ['app.html']);
assertTextSpans(refs, ['title']);
});

Expand Down Expand Up @@ -244,7 +244,7 @@ describe('find references and rename locations', () => {

it('get reference to member in ts file', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
expect(refs.length).toBe(1);

assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['otherTitle']);
Expand Down Expand Up @@ -281,15 +281,15 @@ describe('find references and rename locations', () => {
it('gets reference to member type definition and initialization in component class', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
// 3 references: the type definition, the value assignment, and the read in the template
expect(refs.length).toBe(3);
expect(refs.length).toBe(1);

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

it('gets rename locations in component class', () => {
Expand Down Expand Up @@ -330,9 +330,9 @@ describe('find references and rename locations', () => {

it('get references in ts file', () => {
const refs = getReferencesAtPosition(_('/app.html'), cursor)!;
expect(refs.length).toBe(2);
expect(refs.length).toBe(1);

assertFileNames(refs, ['app.ts', 'app.html']);
assertFileNames(refs, ['app.html']);
assertTextSpans(refs, ['batman']);
});

Expand Down Expand Up @@ -482,8 +482,8 @@ describe('find references and rename locations', () => {

it('should get references', () => {
const refs = getReferencesAtPosition(_('/app.html'), cursor)!;
expect(refs.length).toBe(2);
assertFileNames(refs, ['dir.ts', 'app.html']);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.html']);
assertTextSpans(refs, ['dirValue']);
});

Expand All @@ -508,8 +508,8 @@ describe('find references and rename locations', () => {

it('should get references', () => {
const refs = getReferencesAtPosition(_('/app.html'), cursor)!;
expect(refs.length).toBe(2);
assertFileNames(refs, ['dir.ts', 'app.html']);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.html']);
assertTextSpans(refs, ['dirValue']);
});

Expand All @@ -534,8 +534,8 @@ describe('find references and rename locations', () => {

it('should get references', () => {
const refs = getReferencesAtPosition(_('/app.html'), cursor)!;
expect(refs.length).toBe(2);
assertFileNames(refs, ['dir.ts', 'app.html']);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.html']);
assertTextSpans(refs, ['doSomething']);
});

Expand Down Expand Up @@ -671,8 +671,8 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
assertFileNames(refs, ['app.ts', 'example-directive.ts']);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['identifier']);
});

Expand Down Expand Up @@ -701,7 +701,7 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['name']);
});
Expand Down Expand Up @@ -751,9 +751,9 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(5);
assertFileNames(refs, ['index.d.ts', 'prefix-pipe.ts', 'app.ts']);
assertTextSpans(refs, ['transform', 'prefixPipe']);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['prefixPipe']);
});

it('should find rename locations', () => {
Expand Down Expand Up @@ -787,7 +787,7 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
expect(refs.length).toBe(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['prefix']);
});
Expand Down Expand Up @@ -828,8 +828,8 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toEqual(2);
assertFileNames(refs, ['string-model.ts', 'app.ts']);
expect(refs.length).toEqual(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['model']);
});

Expand Down Expand Up @@ -908,8 +908,8 @@ describe('find references and rename locations', () => {

it('should work for text attributes', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toEqual(2);
assertFileNames(refs, ['string-model.ts', 'app.ts']);
expect(refs.length).toEqual(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['model']);
});

Expand Down Expand Up @@ -949,8 +949,8 @@ describe('find references and rename locations', () => {

it('should work from the TS input declaration', () => {
const refs = getReferencesAtPosition(_('/string-model.ts'), cursor)!;
expect(refs.length).toEqual(2);
assertFileNames(refs, ['app.ts', 'string-model.ts']);
expect(refs.length).toEqual(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['model']);
});

Expand Down Expand Up @@ -1005,8 +1005,8 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/other-dir.ts'), cursor)!;
expect(refs.length).toEqual(3);
assertFileNames(refs, ['app.ts', 'string-model.ts', 'other-dir.ts']);
expect(refs.length).toEqual(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['model']);
});

Expand Down Expand Up @@ -1036,9 +1036,9 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toEqual(2);
assertFileNames(refs, ['string-model.ts', 'app.ts']);
assertTextSpans(refs, ['aliasedModel', 'alias']);
expect(refs.length).toEqual(1);
assertFileNames(refs, ['app.ts']);
assertTextSpans(refs, ['alias']);
});

it('should find rename locations', () => {
Expand Down Expand Up @@ -1090,7 +1090,7 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toEqual(2);
expect(refs.length).toEqual(1);
assertTextSpans(refs, ['modelChange']);
});

Expand All @@ -1115,8 +1115,8 @@ describe('find references and rename locations', () => {

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toEqual(2);
assertTextSpans(refs, ['aliasedModelChange', 'alias']);
expect(refs.length).toEqual(1);
assertTextSpans(refs, ['alias']);
});

it('should find rename locations', () => {
Expand Down Expand Up @@ -1152,12 +1152,9 @@ describe('find references and rename locations', () => {
env = createModuleWithDeclarations([appFile, dirFile]);

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

describe('directives', () => {
Expand Down Expand Up @@ -1191,9 +1188,9 @@ describe('find references and rename locations', () => {
const refs = getReferencesAtPosition(_('/dir.ts'), cursor)!;
// 4 references are: class declaration, template usage, app import and use in declarations
// list.
expect(refs.length).toBe(4);
assertTextSpans(refs, ['<div dir>', 'Dir']);
assertFileNames(refs, ['app.ts', 'dir.ts']);
expect(refs.length).toBe(1);
assertTextSpans(refs, ['<div dir>']);
assertFileNames(refs, ['app.ts']);
});

it('should find rename locations', () => {
Expand Down Expand Up @@ -1241,9 +1238,9 @@ describe('find references and rename locations', () => {

it('gets references to all matching directives', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(8);
assertTextSpans(refs, ['<div dir>', 'Dir', 'Dir2']);
assertFileNames(refs, ['app.ts', 'dir.ts', 'dir2.ts']);
expect(refs.length).toBe(2);
assertTextSpans(refs, ['<div dir>']);
assertFileNames(refs, ['app.ts']);
});

it('finds rename locations for all matching directives', () => {
Expand Down Expand Up @@ -1274,9 +1271,9 @@ describe('find references and rename locations', () => {

it('should be able to request references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(6);
assertTextSpans(refs, ['<div *ngFor="let item of items"></div>', 'NgForOf']);
assertFileNames(refs, ['index.d.ts', 'app.ts']);
expect(refs.length).toBe(1);
assertTextSpans(refs, ['<div *ngFor="let item of items"></div>']);
assertFileNames(refs, ['app.ts']);
});

it('should not support rename if directive is in a dts file', () => {
Expand Down Expand Up @@ -1317,9 +1314,9 @@ describe('find references and rename locations', () => {
const refs = getReferencesAtPosition(_('/comp.ts'), cursor)!;
// 4 references are: class declaration, template usage, app import and use in declarations
// list.
expect(refs.length).toBe(4);
assertTextSpans(refs, ['<my-comp>', 'MyComp']);
assertFileNames(refs, ['app.ts', 'comp.ts']);
expect(refs.length).toBe(1);
assertTextSpans(refs, ['<my-comp>']);
assertFileNames(refs, ['app.ts']);
});

it('gets rename locations', () => {
Expand Down Expand Up @@ -1363,9 +1360,9 @@ describe('find references and rename locations', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
// 4 references are: class declaration, template usage, app import and use in declarations
// list.
expect(refs.length).toBe(4);
assertTextSpans(refs, ['<my-comp>', 'MyComp']);
assertFileNames(refs, ['app.ts', 'comp.ts']);
expect(refs.length).toBe(1);
assertTextSpans(refs, ['<my-comp>']);
assertFileNames(refs, ['app.ts']);
});

it('finds rename locations', () => {
Expand Down

0 comments on commit 77e8d03

Please sign in to comment.