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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
import { Position, Range } from 'vscode-languageserver';
import { SvelteDocumentSnapshot } from './DocumentSnapshot';
import { Document } from '../../lib/documents';
import { SvelteNode } from './svelte-ast-utils';
export class CSSClassDefinitionLocator {
initialNodeAt: SvelteNode;
cssClassTerminators = ['', '{', '.', '>', '~', '>', '[', ':', '#', '+'];
constructor(
public tsDoc: SvelteDocumentSnapshot,
public position: Position,
public document: Document
) {
this.initialNodeAt = this.tsDoc.svelteNodeAt(this.position) as SvelteNode;
}

getCSSClassDefinition() {
Copy link
Member

Choose a reason for hiding this comment

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

I believe it could be more promising/faster to defer CSS parsing to vscode-css-languageservice which has a parse method. The AST that is returned doesn't have type definitions because it's private, but I'm honestly not worried too much about that since it's unlikely to change for this. That AST could be traversed to more easily find the correct node to link to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into this again. Using svelteNodeAt is giving back a lot of good Svelte information that I wouldn't think the vscode-css-languageservice would know about. This method will tell us all the other variations of Svelte css such as whether it is a ConditionalExpression or Class Directive Format.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't mean the HTML part, I meant the CSS part. The code does string traversal of the CSS to find the matching definition, but it could be more promising to use the CSS AST output of vscode-css-languageservice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the while loop in getStandardFormatClassName() ? This is actually not traversing css but the class attribute string itself to figure out which classname the position is at

class="test te|st1 bar foo" so in this example the cursor is in the middle of test1 so that's what we're going to look for in the style section

if (this.isStandardClassFormat()) {
return this.getStandardFormatClassName();
}

if (this.isTargetHTMLElement()) {
return this.getHTMlElementName();
}

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"
*/
private isStandardClassFormat() {
if (this.initialNodeAt?.type == 'Text' && this.initialNodeAt?.parent?.name == 'class') {
return true;
}

return false;
}

/**
* HTML Element target:
* <main>
*/
private isTargetHTMLElement() {
if (this.initialNodeAt?.type == 'Element') {
return true;
}

return false;
}

/**
* Conditional Expression format:
* class="{current === 'baz' ? 'selected' : ''
*/
private 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'}"
*/
private isDirectiveFormat() {
if (this.initialNodeAt?.type == 'Class' && this.initialNodeAt?.parent?.type == 'Element') {
return true;
}

return false;
}

private 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}`);
}

private getHTMlElementName() {
const result = this.getDefinitionRangeWithinStyleSection(`${this.initialNodeAt.name}`);
if (result) {
//Shift start/end to get the highlight right
const originalStart = result.start.character;
result.start.character -= 1;
if (this.initialNodeAt.name) {
result.end.character = originalStart + this.initialNodeAt.name.length;
}

return result;
}
}

private getDefinitionRangeWithinStyleSection(targetClass: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Apologies, the comment on line getCSSClassDefinition was actually meant to appear here. That code does a somewhat unperformant string search. I believe it could be more promising/faster to defer CSS parsing to vscode-css-languageservice which has a parse method. The AST that is returned doesn't have type definitions because it's private, but I'm honestly not worried too much about that since it's unlikely to change for this. That AST could be traversed to more easily find the correct node to link to.

let indexOccurence = this.document.content.indexOf(
targetClass,
this.document.styleInfo?.start
);

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;
} else {
break;
}
}
}

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

return false;
}

private isExactClassMatch(testRange: Range) {
//Check nothing before the test position
if (testRange.start.character > 0) {
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 css terminator 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 (this.cssClassTerminators.includes(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 './CSSClassDefinitionLocator';
import {
AppCompletionItem,
AppCompletionList,
Expand Down Expand Up @@ -334,6 +335,31 @@ export class TypeScriptPlugin
async getDefinitions(document: Document, position: Position): Promise<DefinitionLink[]> {
const { lang, tsDoc } = await this.getLSAndTSDoc(document);

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,
tsDoc.offsetAt(tsDoc.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;
}

type HTMLLike = 'Element' | 'InlineComponent' | 'Body' | 'Window';
Expand Down