Skip to content

Allow loading custom typescript.tsdk settings on web hosts #138211

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.

Sign up for GitHub

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

Closed
weswigham wants to merge 4 commits into from
Closed

Allow loading custom typescript.tsdk settings on web hosts #138211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4125d3
Allow loading custom typescript.tsdk settings on web hosts
weswigham Nov 30, 2021
ebc0de7
Fix quotemarks
weswigham Nov 30, 2021
cd9d653
Move config extraction into base class
weswigham Dec 7, 2021
110896e
Remove node version getters form browser impl
weswigham Dec 7, 2021
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
66 changes: 58 additions & 8 deletions extensions/typescript-language-features/src/extension.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,70 @@ import { PluginManager } from './utils/plugins';

class StaticVersionProvider implements ITypeScriptVersionProvider {

private configuration?: TypeScriptServiceConfiguration;

constructor(
private readonly _version: TypeScriptVersion
private readonly _browserVersion: TypeScriptVersion
) { }

updateConfiguration(_configuration: TypeScriptServiceConfiguration): void {
// noop
updateConfiguration(configuration: TypeScriptServiceConfiguration): void {
this.configuration = configuration;
}

public get defaultVersion(): TypeScriptVersion {
return this.globalVersion || this.bundledVersion;
}

get bundledVersion() { return this._browserVersion; }

public get globalVersion(): TypeScriptVersion | undefined {
if (this.configuration?.globalTsdk) {
const globals = this.loadVersionsFromSetting(TypeScriptVersionSource.UserSetting, this.configuration.globalTsdk);
if (globals && globals.length) {
return globals[0];
}
}
return undefined;
}

get defaultVersion() { return this._version; }
get bundledVersion() { return this._version; }
public get localVersion(): TypeScriptVersion | undefined {
const tsdkVersions = this.localTsdkVersions;
if (tsdkVersions && tsdkVersions.length) {
return tsdkVersions[0];
}
return undefined;
}

public get localVersions(): TypeScriptVersion[] {
const allVersions = this.localTsdkVersions;
const paths = new Set<string>();
return allVersions.filter(x => {
if (paths.has(x.path)) {
return false;
}
paths.add(x.path);
return true;
});
}

readonly globalVersion = undefined;
readonly localVersion = undefined;
readonly localVersions = [];
private get localTsdkVersions(): TypeScriptVersion[] {
const localTsdk = this.configuration?.localTsdk;
return localTsdk ? this.loadVersionsFromSetting(TypeScriptVersionSource.WorkspaceSetting, localTsdk) : [];
}

private loadVersionsFromSetting(source: TypeScriptVersionSource, tsdkPathSetting: string): TypeScriptVersion[] {
if (tsdkPathSetting.startsWith('https://')) {
const serverPath = vscode.Uri.joinPath(vscode.Uri.parse(tsdkPathSetting), 'tsserver.js');
return [
new TypeScriptVersion(source,
serverPath.toString(),
/*DiskTypeScriptVersionProvider.getApiVersion(serverPath)*/ API.fromVersionString('4.4.1'), // TODO: Pull version form URL if possible
Copy link
Member Author

@weswigham weswigham Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right now this API version is hardcoded - we could try and pull it from the URL (which could actually be anything, even a blob, so looking for a x.y.z in the url would be heuristic at best) or prefetch the document and search it for .versionMajorMinor = and pull out the version that way or assume the URL is a full directory structure and walk up to where we'd expect the package.json to be and fetch that. Unsure what we want to do - in either case we'd probably need to asyncify a few getters.

Copy link
Member Author

@weswigham weswigham Nov 30, 2021

Choose a reason for hiding this comment

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

I have a version that doesn't break any of the internal APIs by using a sync XMLHttpRequest to inspect the server's text contents for the version string. Could be OK - definitely would want to know if that or async'ing all these getters would be preferable.

Copy link
Collaborator

@mjbvz mjbvz Dec 1, 2021

Choose a reason for hiding this comment

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

How much work would it be to use an async call? I worry that the sync request will stall all other extension operations

Copy link
Contributor

@orta orta Dec 1, 2021

Choose a reason for hiding this comment

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

Alternative: could it say "Downloading..." and then show the ts sdk version from the API when it's finished loading and easy to access?

Copy link
Member Author

@weswigham weswigham Dec 2, 2021

Choose a reason for hiding this comment

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

How much work would it be to use an async call? I worry that the sync request will stall all other extension operations

All the getters for different versions need to become getters for promises, and all of the functions using those getters need to become async. So pretty much everything version selector related. So long as that restructuring sounds fine, it's doable - just a large structural change for an otherwise pretty simple change. I'll finish up my local version of that for comparison - I originally bailed on it and prepped the sync request one (which shouldn't be that much worse than the sync FS calls in the non browser version) once I got to needing to refactor the getters types and thus existing APIs.

Copy link
Member Author

@weswigham weswigham Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So, I have an implementation with asyncness and lemme tell you: async is toxic. It had to flow out to everywhere.

tsdkPathSetting)
];
}

return [];
}
}

export function activate(
Expand Down
14 changes: 1 addition & 13 deletions extensions/typescript-language-features/src/utils/configuration.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,6 @@
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import * as vscode from 'vscode';
import { BaseServiceConfigurationProvider } from './configuration';

export class BrowserServiceConfigurationProvider extends BaseServiceConfigurationProvider {

// On browsers, we only support using the built-in TS version
protected extractGlobalTsdk(_configuration: vscode.WorkspaceConfiguration): string | null {
return null;
}

protected extractLocalTsdk(_configuration: vscode.WorkspaceConfiguration): string | null {
return null;
}
}
export class BrowserServiceConfigurationProvider extends BaseServiceConfigurationProvider { }
18 changes: 6 additions & 12 deletions extensions/typescript-language-features/src/utils/configuration.electron.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,13 @@ export class ElectronServiceConfigurationProvider extends BaseServiceConfigurati
return inspectValue;
}

protected extractGlobalTsdk(configuration: vscode.WorkspaceConfiguration): string | null {
const inspect = configuration.inspect('typescript.tsdk');
if (inspect && typeof inspect.globalValue === 'string') {
return this.fixPathPrefixes(inspect.globalValue);
}
return null;
protected override extractGlobalTsdk(configuration: vscode.WorkspaceConfiguration): string | null {
const result = super.extractGlobalTsdk(configuration);
return result && this.fixPathPrefixes(result);
}

protected extractLocalTsdk(configuration: vscode.WorkspaceConfiguration): string | null {
const inspect = configuration.inspect('typescript.tsdk');
if (inspect && typeof inspect.workspaceValue === 'string') {
return this.fixPathPrefixes(inspect.workspaceValue);
}
return null;
protected override extractLocalTsdk(configuration: vscode.WorkspaceConfiguration): string | null {
const result = super.extractLocalTsdk(configuration);
return result && this.fixPathPrefixes(result);
}
}
17 changes: 15 additions & 2 deletions extensions/typescript-language-features/src/utils/configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,21 @@ export abstract class BaseServiceConfigurationProvider implements ServiceConfigu
};
}

protected abstract extractGlobalTsdk(configuration: vscode.WorkspaceConfiguration): string | null;
protected abstract extractLocalTsdk(configuration: vscode.WorkspaceConfiguration): string | null;
protected extractGlobalTsdk(configuration: vscode.WorkspaceConfiguration): string | null {
const inspect = configuration.inspect('typescript.tsdk');
if (inspect && typeof inspect.globalValue === 'string') {
return inspect.globalValue;
}
return null;
}

protected extractLocalTsdk(configuration: vscode.WorkspaceConfiguration): string | null {
const inspect = configuration.inspect('typescript.tsdk');
if (inspect && typeof inspect.workspaceValue === 'string') {
return inspect.workspaceValue;
}
return null;
}

protected readTsServerLogLevel(configuration: vscode.WorkspaceConfiguration): TsServerLogLevel {
const setting = configuration.get<string>('typescript.tsserver.log', 'off');
Expand Down