Skip to content

html-language-features: text document provider support for customData.html #137557

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

Merged

Conversation

sijakret
Copy link
Contributor

This PR implements #135459
It adds the ability for for html-language-features extension to receive custom-data via Text Document Provider and update on changes to these documents.
With the change extensions can provide contribute dynamic data via customData.html;

@ghost
Copy link

ghost commented Nov 19, 2021

CLA assistant check
All CLA requirements met.

@sijakret sijakret changed the title Html language server/virtual doc support html-language-features: text document provider support for customData.html Nov 20, 2021
return {
get uris() {
return pathsInWorkspace.concat(pathsInExtensions);
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the benefit of that change?

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 converted the path path lists in to Set so the check in line 31 becomes as fast as possible.
With the change, checking of a path has changed should be O(1) but returning the list has higher complexity.
If we want to have a Set to track the paths and avoid having to flatten it to an array in the getter get uris() we would need to maintain both a set and an array in parallel. One for filtering paths and one to return it. I can change that if you want to. If it makes sense also depends a bit on how often the getter is invoked, but I guess we don't know that..

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I get it, thanks!

@@ -20,34 +22,34 @@ export namespace FsReadDirRequest {

export function serveFileSystemRequests(client: CommonLanguageClient, runtime: Runtime, subscriptions: { dispose(): any }[]) {
subscriptions.push(client.onRequest(FsContentRequest.type, (param: { uri: string; encoding?: string; }) => {
const uri = Uri.parse(param.uri);
if (uri.scheme === 'file') {
const uri = param.uri.match(uriScheme);
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on earlier comments I thought Uri.parse shall explicitly NOT be used to check for schemes.
I actually actually liked it better before but it feels a bit inconsistent to use sometimes use a bare regex and sometimes use Uri.parse. Don't have a string opinion here. I can change it back. Lat me know what you prefer.

One more important thing:
The serveFileSystemRequests(..) helper was already in the code, I did not create it.
However, it was unused - so someone had written that helper but never activated it (or deactivated it for some reason).
I just activated added in client.onReady() in htmlClient.ts (same way it is done in css-language-features where pretty much the same code has been active all along).
Was there a reason why these handlers were added but commented out? Seems a bit suspicious..

Copy link
Contributor

Choose a reason for hiding this comment

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

uti is always in the URI format. So Uri.parse is ok to use.

The point is to no use Uri.parse on a relative path.

if (uri.scheme === 'file') {
// only resolve file paths relative to extension
if (!uriScheme.test(rp)) {
// no schame -> resolve relative to extension
Copy link
Contributor

Choose a reason for hiding this comment

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

typo on 'scheme'

@aeschli
Copy link
Contributor

aeschli commented Nov 26, 2021

I made some polish changes (and reverted the requestService change), so this looks all good now.

The missing serveFileSystemRequests was a bug and all that is unfinished work of myself. I'll work on that when after the PR is merged.

@aeschli aeschli merged commit c67fd6e into microsoft:main Nov 26, 2021
@aeschli
Copy link
Contributor

aeschli commented Nov 26, 2021

@sijakret Thanks for all the work!

@sijakret
Copy link
Contributor Author

sijakret commented Nov 27, 2021

@aeschli something got lost! sijakret@f1455ea#diff-35b696cf6cdc005ac94347afd4160dbae29c3a336d5ce690501bbd67ec6fddbcL35

Edit: I have created another small PR with a fix: #137980

guibber pushed a commit to guibber/vscode that referenced this pull request Nov 30, 2021
…r/virtual-doc-support

html-language-features: text document provider support for customData.html
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants