Skip to content

(feat)Go to definition from class in template to css #1518

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
6 changes: 5 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
{
"typescript.preferences.quoteStyle": "single"
"typescript.preferences.quoteStyle": "single",
"files.exclude": {
"**/**/dist": true,
"**/**/node_modules": true
}
}
1 change: 1 addition & 0 deletions packages/language-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"typings": "dist/src/index",
"scripts": {
"test": "cross-env TS_NODE_TRANSPILE_ONLY=true mocha --require ts-node/register \"test/**/*.ts\" --exclude \"test/**/*.d.ts\"",
"TypescriptPlugin": "cross-env TS_NODE_TRANSPILE_ONLY=true mocha --require ts-node/register \"test/**/TypescriptPlugin.test.ts\" --exclude \"test/**/*.d.ts\"",
"build": "tsc",
"prepublishOnly": "npm run build",
"watch": "tsc -w"
Expand Down
167 changes: 167 additions & 0 deletions packages/language-server/src/plugins/css/CSSClassDefinitionLocator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
import { Position, Range } from 'vscode-languageserver';
import { SvelteDocumentSnapshot } from '../typescript/DocumentSnapshot';
import { Document } from '../../lib/documents';
import { SvelteNode } from '../typescript/svelte-ast-utils';
export class CSSClassDefinitionLocator {
initialNodeAt: SvelteNode;
constructor(
public tsDoc: SvelteDocumentSnapshot,
public position: Position,
public document: Document
) {
this.initialNodeAt = this.tsDoc.svelteNodeAt(this.position) as SvelteNode;
}

public GetCSSClassDefinition() {
if (this.IsStandardClassFormat()) {
return this.GetStandardFormatClassName();
}

if (this.IsDirectiveFormat() && this.initialNodeAt.name) {
return this.GetDefinitionRangeWithinStyleSection(`.${this.initialNodeAt.name}`);
}

if (this.IsConditionalExpressionClassFormat() && this.initialNodeAt.value) {
return this.GetDefinitionRangeWithinStyleSection(`.${this.initialNodeAt.value}`);
}

return false;
}

/**
* Standard format:
* class="test test1"
*/
public IsStandardClassFormat() {
if (this.initialNodeAt?.type == 'Text' && this.initialNodeAt?.parent?.name == 'class') {
return true;
}

return false;
}

/**
* Conditional Expression format:
* class="{current === 'baz' ? 'selected' : ''
*/
public IsConditionalExpressionClassFormat() {
if (
this.initialNodeAt?.type == 'Literal' &&
this.initialNodeAt?.parent?.type == 'ConditionalExpression' &&
this.initialNodeAt?.parent.parent?.parent?.name == 'class'
) {
return true;
}

return false;
}

/**
* Class Directive format:
* class:active="{current === 'foo'}"
*/
public IsDirectiveFormat() {
if (this.initialNodeAt?.type == 'Class' && this.initialNodeAt?.parent?.type == 'Element') {
return true;
}

return false;
}

public GetStandardFormatClassName() {
const testEndTagRange = Range.create(
Position.create(this.position.line, 0),
Position.create(this.position.line, this.position.character)
);
const text = this.document.getText(testEndTagRange);

let loopLength = text.length;
let testPosition = this.position.character;
let spaceCount = 0;

//Go backwards until hitting a " and keep track of how many spaces happened along the way
while (loopLength) {
const testEndTagRange = Range.create(
Position.create(this.position.line, testPosition - 1),
Position.create(this.position.line, testPosition)
);
const text = this.document.getText(testEndTagRange);
if (text === ' ') {
spaceCount++;
}

if (text === '"') {
break;
}

testPosition--;
loopLength--;
}

const cssClassName = this.initialNodeAt?.data.split(' ')[spaceCount];

return this.GetDefinitionRangeWithinStyleSection(`.${cssClassName}`);
}

public GetDefinitionRangeWithinStyleSection(targetClass: string) {
let indexOccurence = this.document.content.indexOf(targetClass, 0);

while (indexOccurence >= 0) {
if (this.IsOffsetWithinStyleSection(indexOccurence)) {
const startPosition = this.document.positionAt(indexOccurence);
const targetRange = Range.create(
startPosition,
Position.create(
startPosition.line,
startPosition.character + targetClass.length
)
);
indexOccurence = this.document.content.indexOf(targetClass, indexOccurence + 1);

if (!this.IsExactClassMatch(targetRange)) {
continue;
}

return targetRange;
}
}
}

public IsOffsetWithinStyleSection(offsetPosition: number) {
if (this.document.styleInfo) {
if (
offsetPosition > this.document.styleInfo?.start &&
offsetPosition < this.document.styleInfo?.end
) {
return true;
}
}

return false;
}

public IsExactClassMatch(testRange: Range) {
//Check nothing before the test position
if (testRange.start.character > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This would fail for something like .foo.bar { .. } or .foo .bar { .. }. In general it's a little brittle to only use text matching here. The vscode-css-language-service which is used in the CSSPlugin provides an AST I think, but it's private. This puts us at risk of breaking changes but I feel it's more robust to use that one instead and fall back to this text-based matching only if parsing fails (because a different preprocessor is used for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a feeling about that one. The case I thought would be the most straightforward turned out, not. I will try to improve it. I was looking for a way to pull the text for an entire line but couldn't find it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that svelte doesn't recognize the .foo .bar case either

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dummdidumm So this actually did already work for .foo .bar but did not for .foo.bar

Try the latest, it handles all sorts of combinations. I feel this is a good compromise especially since I really don't feel that style's inside a svelte component would/should be very complex. If after testing you still feel like we need more horsepower I can look into the CSS AST(just point me in the right direction for that)

const beforerange = Range.create(
Position.create(testRange.start.line, testRange.start.character - 1),
Position.create(testRange.start.line, testRange.start.character)
);
if (this.document.getText(beforerange).trim()) {
return false;
}
}

//Check space or { is after the test position
const afterRange = Range.create(
Position.create(testRange.end.line, testRange.end.character),
Position.create(testRange.end.line, testRange.end.character + 1)
);
const afterRangeText = this.document.getText(afterRange).trim();
if (afterRangeText == '' || afterRangeText == '{') {
return true;
}

return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
import { Document, getTextInRange, mapSymbolInformationToOriginal } from '../../lib/documents';
import { LSConfigManager, LSTypescriptConfig } from '../../ls-config';
import { isNotNullOrUndefined, isZeroLengthRange, pathToUrl } from '../../utils';
import { CSSClassDefinitionLocator } from '../css/CSSClassDefinitionLocator';
import {
AppCompletionItem,
AppCompletionList,
Expand Down Expand Up @@ -329,6 +330,31 @@ export class TypeScriptPlugin
const { lang, tsDoc } = await this.getLSAndTSDoc(document);
const mainFragment = tsDoc.getFragment();

const cssClassHelper = new CSSClassDefinitionLocator(tsDoc, position, document);
Copy link
Member

Choose a reason for hiding this comment

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

The idea of the plugin folders is that they are mostly independent of each other, so it would be better to put this logic into the CSSPlugin or move the class into the TypeScript plugin folder - but I feel like the best place would be SveltePlugin.ts. This brings us to a question to which I don't have a good answer yet: What is the best place for code like this? It's cross-cutting so it's not obvious to me in which plugin this does belong. Maybe this hints at that the plugin system needs a redesign / needs to be replaced with something new, but that's for another day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was having trouble where to place this. I moved to TypeScript folder for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to relocate wherever you feel is best

const cssDefinitionRange = cssClassHelper.GetCSSClassDefinition();
if (cssDefinitionRange) {
const results: DefinitionLink[] = [];
cssDefinitionRange.start.character++; //Report start of name instead of start at . for easy rename (F2) possibilities

const originRange = Range.create(
Position.create(position.line, position.character),
Position.create(position.line, position.character)
);

results.push(
LocationLink.create(
pathToUrl(document.getFilePath() as string),
cssDefinitionRange,
cssDefinitionRange,
originRange
)
);

if (results) {
return results;
Copy link
Member

Choose a reason for hiding this comment

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

The results should probably be merged with what else comes up. In the case of class:foo both .foo { ... } in <style> and let foo = ... in script are definitons, and we would lose the latter when returning early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow. In all the tests and variations I've used it seems to work. When you hit F12 to go to definition there can only be 1 result (if a definition can be found).

}
}

const defs = lang.getDefinitionAndBoundSpan(
tsDoc.filePath,
mainFragment.offsetAt(mainFragment.getGeneratedPosition(position))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ export interface SvelteNode {
end: number;
type: string;
parent?: SvelteNode;
value?: any;
data?: any;
name?: string;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,132 @@ function test(useNewTransformation: boolean) {
]);
}

it('provides standard css definition from svelte template', async () => {
const { plugin, document } = setup('css-definitions.svelte');

const definitions = await plugin.getDefinitions(document, Position.create(12, 19));

assert.deepStrictEqual(definitions, [
{
targetRange: {
start: {
line: 22,
character: 3
},
end: {
line: 22,
character: 8
}
},
targetSelectionRange: {
start: {
line: 22,
character: 3
},
end: {
line: 22,
character: 8
}
},
originSelectionRange: {
start: {
line: 12,
character: 19
},
end: {
line: 12,
character: 19
}
},
targetUri: getUri('css-definitions.svelte')
}
]);
});

it('provides conditional expression css definition from svelte template', async () => {
const { plugin, document } = setup('css-definitions.svelte');

const definitions = await plugin.getDefinitions(document, Position.create(16, 33));

assert.deepStrictEqual(definitions, [
{
targetRange: {
start: {
line: 50,
character: 3
},
end: {
line: 50,
character: 11
}
},
targetSelectionRange: {
start: {
line: 50,
character: 3
},
end: {
line: 50,
character: 11
}
},
originSelectionRange: {
start: {
line: 16,
character: 33
},
end: {
line: 16,
character: 33
}
},
targetUri: getUri('css-definitions.svelte')
}
]);
});

it('provides class directive css definition from svelte template', async () => {
const { plugin, document } = setup('css-definitions.svelte');

const definitions = await plugin.getDefinitions(document, Position.create(12, 31));

assert.deepStrictEqual(definitions, [
{
targetRange: {
start: {
line: 45,
character: 3
},
end: {
line: 45,
character: 9
}
},
targetSelectionRange: {
start: {
line: 45,
character: 3
},
end: {
line: 45,
character: 9
}
},
originSelectionRange: {
start: {
line: 12,
character: 31
},
end: {
line: 12,
character: 31
}
},
targetUri: getUri('css-definitions.svelte')
}
]);
});

it('(within script simple)', async () => {
await test$StoreDef(
Position.create(9, 1),
Expand Down
Loading