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: implement webview to the api docs as go to definition #496

Open
wants to merge 17 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
"name": "Launch Client",
"runtimeExecutable": "${execPath}",
"args": [
"--extensionDevelopmentPath=${workspaceRoot}/packages/vscode-ui5-language-assistant"
"--extensionDevelopmentPath=${workspaceRoot}/packages/vscode-ui5-language-assistant",
"--disable-extensions"
],
"outFiles": [
"${workspaceRoot}/packages/vscode-ui5-language-assistant/lib/src/**/*.js"
Expand Down
9 changes: 8 additions & 1 deletion packages/language-server/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
* Absolute path to the server's "main" module
* This is useful when launching the server in a separate process (e.g via spawn).
*/
import { LogLevel } from "@vscode-logging/types";

import { LogLevel } from "@vscode-logging/types";
export declare const SERVER_PATH: string;

export type ServerInitializationOptions = {
Expand All @@ -25,6 +25,13 @@ export type ServerInitializationOptions = {
logLevel?: LogLevel;
};

export function getNodeName(
Copy link
Member

Choose a reason for hiding this comment

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

why does the language server exposes a logical utility function?
It generally runs in a different process and communicates via JSON_RPC

Copy link
Member

Choose a reason for hiding this comment

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

Generally the language server should use logical utilities from other packages.
Maybe getNodeName should be extracted out of the language server sub-package.

text: string,
Copy link
Member

Choose a reason for hiding this comment

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

with so many arguments, some of which are optional you should probably use a config object in this case.

offsetAt: number,
cachePath?: string,
framework?: string,
ui5Version?: string
): Promise<string | undefined>;
export type FetchResponse = {
ok: boolean;
status: number;
Expand Down
23 changes: 23 additions & 0 deletions packages/language-server/src/api.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { resolve } from "path";
import { existsSync } from "fs";
import { getNodeDetail, getUI5NodeName } from "./documentation";

// for use in productive flows
const bundledPath = resolve(__dirname, "..", "..", "dist", "server.js");
Expand All @@ -19,3 +20,25 @@ export const SERVER_PATH: string = isDevelopmentRun
? /* istanbul ignore else - no tests (yet?) on bundled artifacts */
sourcesPath
: bundledPath;

export async function getNodeName(
text: string,
offsetAt: number,
cachePath?: string | undefined,
framework?: string | undefined,
ui5Version?: string | undefined
): Promise<string | undefined> {
const ui5Node = await getUI5NodeName(
offsetAt,
text,
undefined,
cachePath,
framework,
ui5Version
);
return ui5Node
Copy link
Member

Choose a reason for hiding this comment

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

I think too much is done here in a single expression.
Can you split this up into multiple statements for readability?

? ui5Node.kind !== "UI5Class"
? `${ui5Node.parent?.library}.${ui5Node.parent?.name}`
: getNodeDetail(ui5Node)
: undefined;
}
32 changes: 32 additions & 0 deletions packages/language-server/src/documentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ import {
getLink,
} from "@ui5-language-assistant/logic-utils";
import { GENERATED_LIBRARY } from "@ui5-language-assistant/semantic-model";
import { DocumentCstNode, parse } from "@xml-tools/parser";
import { buildAst } from "@xml-tools/ast";
import { astPositionAtOffset } from "@xml-tools/ast-position";
import { findUI5HoverNodeAtOffset } from "@ui5-language-assistant/xml-views-tooltip";
import { getSemanticModel } from "./ui5-model";

export function getNodeDocumentation(
node: BaseUI5Node,
Expand Down Expand Up @@ -108,3 +113,30 @@ export function getNodeDetail(node: BaseUI5Node): string {
return node.name;
}
}

export async function getUI5NodeName(
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this function belongs in this sub-package

offsetAt: number,
text: string,
model?: UI5SemanticModel,
cachePath?: string,
framework?: string,
ui5Version?: string
): Promise<BaseUI5Node | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

with so many params and optional params a config object may be warranted

const documentText = text;
const { cst, tokenVector } = parse(documentText);
Copy link
Member

Choose a reason for hiding this comment

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

Again I do not recall.
Does every flow functional flow re-parses the text each time?
Or is there a cache of documents / CST?

const ast = buildAst(cst as DocumentCstNode, tokenVector);
const offset = offsetAt;
const astPosition = astPositionAtOffset(ast, offset);
if (!model) {
model = await fetchModel(cachePath, framework, ui5Version);
}
if (astPosition !== undefined) {
return findUI5HoverNodeAtOffset(astPosition, model);
}
return undefined;
}

async function fetchModel(cachePath, framework, ui5Version) {
const ui5Model = await getSemanticModel(cachePath, framework, ui5Version);
Copy link
Member

Choose a reason for hiding this comment

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

I do not recall:
Does every functional flow invoke getSemanticModel ?
Or does it happen once on language server startup?

Is this different or the same as other functional flows?

return ui5Model;
}
33 changes: 15 additions & 18 deletions packages/language-server/src/hover.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,30 @@ import {
MarkupContent,
MarkupKind,
} from "vscode-languageserver";
import { parse, DocumentCstNode } from "@xml-tools/parser";
import { buildAst } from "@xml-tools/ast";
import { astPositionAtOffset } from "@xml-tools/ast-position";
import {
UI5SemanticModel,
BaseUI5Node,
} from "@ui5-language-assistant/semantic-model-types";
import { findUI5HoverNodeAtOffset } from "@ui5-language-assistant/xml-views-tooltip";
import { getNodeDocumentation, getNodeDetail } from "./documentation";
import {
getNodeDocumentation,
getNodeDetail,
getUI5NodeName,
} from "./documentation";
import { track } from "./swa";

export function getHoverResponse(
export async function getHoverResponse(
Copy link
Member

Choose a reason for hiding this comment

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

could changing this to async break anything?

model: UI5SemanticModel,
textDocumentPosition: TextDocumentPositionParams,
document: TextDocument
): Hover | undefined {
const documentText = document.getText();
const { cst, tokenVector } = parse(documentText);
const ast = buildAst(cst as DocumentCstNode, tokenVector);
const offset = document.offsetAt(textDocumentPosition.position);
const astPosition = astPositionAtOffset(ast, offset);
if (astPosition !== undefined) {
const ui5Node = findUI5HoverNodeAtOffset(astPosition, model);
if (ui5Node !== undefined) {
track("XML_UI5_DOC_HOVER", ui5Node.kind);
return transformToLspHover(ui5Node, model);
}
): Promise<Hover | undefined> {
const ui5Node = await getUI5NodeName(
document.offsetAt(textDocumentPosition.position),
document.getText(),
model
);
if (ui5Node !== undefined) {
track("XML_UI5_DOC_HOVER", ui5Node.kind);
return transformToLspHover(ui5Node, model);
}

return undefined;
Expand Down
8 changes: 8 additions & 0 deletions packages/language-server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ connection.onInitialize((params: InitializeParams) => {
commands.QUICK_FIX_STABLE_ID_FILE_ERRORS.name,
],
},
referencesProvider: false,
definitionProvider: true,
},
};
});
Expand Down Expand Up @@ -128,6 +130,7 @@ connection.onCompletion(
minUI5Version
);
connection.sendNotification("UI5LanguageAssistant/ui5Model", {
cachePath: initializationOptions?.modelCachePath,
Copy link
Member

Choose a reason for hiding this comment

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

why does this feature require adding the cache path to the notifications?
Perhaps I am rusty in regards to this code base, but why was it not needed for other features, but needed here?

url: getCDNBaseUrl(framework, model.version),
framework,
version: model.version,
Expand Down Expand Up @@ -172,6 +175,7 @@ connection.onHover(
minUI5Version
);
connection.sendNotification("UI5LanguageAssistant/ui5Model", {
cachePath: initializationOptions?.modelCachePath,
url: getCDNBaseUrl(framework, ui5Model.version),
framework,
version: ui5Model.version,
Expand Down Expand Up @@ -226,6 +230,8 @@ documents.onDidChangeContent(async (changeEvent) => {
minUI5Version
);
connection.sendNotification("UI5LanguageAssistant/ui5Model", {
cachePath: initializationOptions?.modelCachePath,

url: getCDNBaseUrl(framework, ui5Model.version),
framework,
version: ui5Model.version,
Expand Down Expand Up @@ -258,6 +264,8 @@ connection.onCodeAction(async (params) => {
minUI5Version
);
connection.sendNotification("UI5LanguageAssistant/ui5Model", {
cachePath: initializationOptions?.modelCachePath,

url: getCDNBaseUrl(framework, ui5Model.version),
framework,
version: ui5Model.version,
Expand Down
89 changes: 88 additions & 1 deletion packages/language-server/test/documentation-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,16 @@ import {
} from "@ui5-language-assistant/test-utils";
import { generate } from "@ui5-language-assistant/semantic-model";
import { UI5SemanticModel } from "@ui5-language-assistant/semantic-model-types";
import { getNodeDocumentation } from "../src/documentation";
import {
getNodeDetail,
getNodeDocumentation,
getUI5NodeName,
} from "../src/documentation";
import { dir as tempDir } from "tmp-promise";

import { Position } from "vscode-languageserver";
import { TextDocument } from "vscode-languageserver-textdocument";
import { getSemanticModel } from "../src/ui5-model";

describe("The @ui5-language-assistant/language-server <getNodeDocumentation> function", () => {
let ui5SemanticModel: UI5SemanticModel;
Expand Down Expand Up @@ -89,5 +98,83 @@ describe("The @ui5-language-assistant/language-server <getNodeDocumentation> fun
const result = getNodeDocumentation(ui5Enum, ui5SemanticModel);
expect(result.value).to.include("Experimental.");
});

it("get the UI5 node name with a model", async () => {
const xmlSnippet = `<mvc:View displayBlock="true"
xmlns="sap.m"
xmlns:pwd="openui5.password"
xmlns:mvc="sap.ui.core.mvc">
<Page>
<content>
<Input liveChange="onInput" />
</content>
</Page>
</mvc:View>`;
const { document, position } = getXmlSnippet(xmlSnippet);

const result = await getUI5NodeName(
document.offsetAt(position),
document.getText(),
ui5SemanticModel
);
if (result) {
Copy link
Member

Choose a reason for hiding this comment

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

why is the test none deterministic ?
why is an if / else needed?

expect(getNodeDetail(result)).to.equal("sap.m.Input");
} else {
expect(result).to.equal("sap.m.Input");
}
});

it("get the UI5 node name without a model", async () => {
const xmlSnippet = `<mvc:View displayBlock="true"
Copy link
Member

Choose a reason for hiding this comment

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

these snippets normally need a "⇶" symbol to mark the cursor position.
Why is this not needed here?

xmlns="sap.m"
xmlns:pwd="openui5.password"
xmlns:mvc="sap.ui.core.mvc">
<Page>
<content>
<Input liveChange="onInput" />
</content>
</Page>
</mvc:View>`;

const cachePath = await tempDir();
const ui5Model = await getSemanticModel(
cachePath.path,
"SAPUI5",
undefined,
true
);

const { document, position } = getXmlSnippet(xmlSnippet);

const result = await getUI5NodeName(
document.offsetAt(position),
document.getText(),
undefined,
cachePath.path,
"SAPUI5",
ui5Model.version
);
if (result) expect(getNodeDetail(result)).to.equal("sap.m.Input");
Copy link
Member

Choose a reason for hiding this comment

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

why is the test none deterministic ?
why is an if / else needed?

else {
expect(result).to.equal("sap.m.Input");
}
});
});

function getXmlSnippet(
Copy link
Member

@bd82 bd82 Oct 27, 2022

Choose a reason for hiding this comment

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

are these two utility test functions copy / pasted from another sub-package?

xmlSnippet: string
): { document: TextDocument; position: Position } {
const xmlText = xmlSnippet.replace("⇶", "");
const offset = xmlSnippet.indexOf("Input");
const document: TextDocument = createTextDocument("xml", xmlText);
const position: Position = document.positionAt(offset);
return { document, position };
}

function createTextDocument(
languageId: string,
content: string
): TextDocument {
return TextDocument.create("uri", languageId, 0, content);
}
});
Loading