Skip to content

Commit

Permalink
fix(client,server): Correctly use bundled version unless tsdk is spec…
Browse files Browse the repository at this point in the history 
…ified (#1970)

* refactor(client): Move server options to client class

This means not having to pass context around and will also give those functions
access to other things directly like outputChannel for logging.

* refactor(client): Log warning to the output channel for when block syntax is disabled

We should log this somewhere or there would be no way to know when this happened to a user.

* refactor(client): Inline server options rather than helper function

A function with a flag is frequently a code smell and in this case it
is. There isn't much code shared between the two modes so inlining is
better.

* fix(client,server): Correctly use bundled version unless tsdk is specified

When the `typescript.tsdk` is empty, it should be ignored. Instead, the
logic previously would _always_ add an empty string tsdk to the
beginning of the probe locations. This would result in resolving the ts
server from the user's node modules rather than prefering the bundled
version as intended unless a different tsdk is specifically set in the
config.

Additionally, if the workspace has many roots, the first one would be
seen as a tsdk. While this would probably still result in the correct
behavior, the code is much easier to follow when the tsdk is a separate
option passed to the server.

* refactor(server): Update tsserver version log to reflect actual version used

The version that's currently logged is whatever the resolve logic gives. However,
this resolve logic actually only affects the version when the banner is used
to override the require statement. This won't happen when debugging the extension
and stumped me for quite some time since the log version was using the tsdk
I specified but I was not seeing the behavior I expected with that version
(I was instead seeing the behavior expected from the bundled version).

(cherry picked from commit 984ecf6)
  • Loading branch information
@atscott
atscott committed Nov 13, 2023
1 parent d8689be commit 46b9a56
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 103 deletions.
185 changes: 92 additions & 93 deletions client/src/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,13 +217,47 @@ export class AngularLanguageClient implements vscode.Disposable {
throw new Error(`An existing client is running. Call stop() first.`);
}

// Node module for the language server
const args = this.constructArgs();
const prodBundle = this.context.asAbsolutePath('server');
const devBundle =
this.context.asAbsolutePath(path.join('bazel-bin', 'server', 'src', 'server.js'));

// If the extension is launched in debug mode then the debug server options are used
// Otherwise the run options are used
const serverOptions: lsp.ServerOptions = {
run: getServerOptions(this.context, false /* debug */),
debug: getServerOptions(this.context, true /* debug */),
run: {
module: this.context.asAbsolutePath('server'),
transport: lsp.TransportKind.ipc,
args,
},
debug: {
// VS Code Insider launches extensions in debug mode by default but users
// install prod bundle so we have to check whether dev bundle exists.
module: fs.existsSync(devBundle) ? devBundle : prodBundle,
transport: lsp.TransportKind.ipc,
options: {
// Argv options for Node.js
execArgv: [
// do not lazily evaluate the code so all breakpoints are respected
'--nolazy',
// If debugging port is changed, update .vscode/launch.json as well
'--inspect=6009',
],
env: {
NG_DEBUG: true,
}
},
args
},
};

if (!extensionVersionCompatibleWithAllProjects(serverOptions.run.module)) {
vscode.window.showWarningMessage(
`A project in the workspace is using a newer version of Angular than the language service extension. ` +
`This may cause the extension to show incorrect diagnostics.`);
}

// Create the language client and start the client.
const forceDebug = process.env['NG_DEBUG'] === 'true';
this.client = new lsp.LanguageClient(
Expand All @@ -242,6 +276,62 @@ export class AngularLanguageClient implements vscode.Disposable {
this.disposables.push(registerNotificationHandlers(this.client));
}

/**
* Construct the arguments that's used to spawn the server process.
* @param ctx vscode extension context
*/
private constructArgs(): string[] {
const config = vscode.workspace.getConfiguration();
const args: string[] = ['--logToConsole'];

const ngLog: string = config.get('angular.log', 'off');
if (ngLog !== 'off') {
// Log file does not yet exist on disk. It is up to the server to create the file.
const logFile = path.join(this.context.logUri.fsPath, 'nglangsvc.log');
args.push('--logFile', logFile);
args.push('--logVerbosity', ngLog);
}

const ngProbeLocations = getProbeLocations(this.context.extensionPath);
args.push('--ngProbeLocations', ngProbeLocations.join(','));

const includeAutomaticOptionalChainCompletions =
config.get<boolean>('angular.suggest.includeAutomaticOptionalChainCompletions');
if (includeAutomaticOptionalChainCompletions) {
args.push('--includeAutomaticOptionalChainCompletions');
}

const includeCompletionsWithSnippetText =
config.get<boolean>('angular.suggest.includeCompletionsWithSnippetText');
if (includeCompletionsWithSnippetText) {
args.push('--includeCompletionsWithSnippetText');
}

const angularVersions = getAngularVersionsInWorkspace();
// Only disable block syntax if we find angular/core and every one we find does not support
// block syntax
if (angularVersions.size > 0 && Array.from(angularVersions).every(v => v.version.major < 17)) {
args.push('--disableBlockSyntax');
this.outputChannel.appendLine(
`All workspace roots are using versions of Angular that do not support control flow block syntax.` +
` Block syntax parsing in templates will be disabled.`);
}

const forceStrictTemplates = config.get<boolean>('angular.forceStrictTemplates');
if (forceStrictTemplates) {
args.push('--forceStrictTemplates');
}

const tsdk = config.get('typescript.tsdk', '');
if (tsdk.trim().length > 0) {
args.push('--tsdk', tsdk);
}
const tsProbeLocations = [...getProbeLocations(this.context.extensionPath)];
args.push('--tsProbeLocations', tsProbeLocations.join(','));

return args;
}

/**
* Kill the language client and perform some clean ups.
*/
Expand Down Expand Up @@ -401,97 +491,6 @@ function getProbeLocations(bundled: string): string[] {
return locations;
}

/**
* Construct the arguments that's used to spawn the server process.
* @param ctx vscode extension context
*/
function constructArgs(ctx: vscode.ExtensionContext): string[] {
const config = vscode.workspace.getConfiguration();
const args: string[] = ['--logToConsole'];

const ngLog: string = config.get('angular.log', 'off');
if (ngLog !== 'off') {
// Log file does not yet exist on disk. It is up to the server to create the file.
const logFile = path.join(ctx.logUri.fsPath, 'nglangsvc.log');
args.push('--logFile', logFile);
args.push('--logVerbosity', ngLog);
}

const ngProbeLocations = getProbeLocations(ctx.extensionPath);
args.push('--ngProbeLocations', ngProbeLocations.join(','));

const includeAutomaticOptionalChainCompletions =
config.get<boolean>('angular.suggest.includeAutomaticOptionalChainCompletions');
if (includeAutomaticOptionalChainCompletions) {
args.push('--includeAutomaticOptionalChainCompletions');
}

const includeCompletionsWithSnippetText =
config.get<boolean>('angular.suggest.includeCompletionsWithSnippetText');
if (includeCompletionsWithSnippetText) {
args.push('--includeCompletionsWithSnippetText');
}

const angularVersions = getAngularVersionsInWorkspace();
// Only disable block syntax if we find angular/core and every one we find does not support block
// syntax
if (angularVersions.size > 0 && Array.from(angularVersions).every(v => v.version.major < 17)) {
args.push('--disableBlockSyntax');
}

const forceStrictTemplates = config.get<boolean>('angular.forceStrictTemplates');
if (forceStrictTemplates) {
args.push('--forceStrictTemplates');
}

const tsdk: string|null = config.get('typescript.tsdk', null);
const tsProbeLocations = [tsdk, ...getProbeLocations(ctx.extensionPath)];
args.push('--tsProbeLocations', tsProbeLocations.join(','));

return args;
}

function getServerOptions(ctx: vscode.ExtensionContext, debug: boolean): lsp.NodeModule {
// Environment variables for server process
const prodEnv = {};
const devEnv = {
...prodEnv,
NG_DEBUG: true,
};

// Node module for the language server
const args = constructArgs(ctx);
const prodBundle = ctx.asAbsolutePath('server');
const devBundle = ctx.asAbsolutePath(path.join('bazel-bin', 'server', 'src', 'server.js'));
// VS Code Insider launches extensions in debug mode by default but users
// install prod bundle so we have to check whether dev bundle exists.
const latestServerModule = debug && fs.existsSync(devBundle) ? devBundle : prodBundle;

if (!extensionVersionCompatibleWithAllProjects(latestServerModule)) {
vscode.window.showWarningMessage(
`A project in the workspace is using a newer version of Angular than the language service extension. ` +
`This may cause the extension to show incorrect diagnostics.`);
}

// Argv options for Node.js
const prodExecArgv: string[] = [];
const devExecArgv: string[] = [
// do not lazily evaluate the code so all breakpoints are respected
'--nolazy',
// If debugging port is changed, update .vscode/launch.json as well
'--inspect=6009',
];

return {
module: latestServerModule,
transport: lsp.TransportKind.ipc,
args,
options: {
env: debug ? devEnv : prodEnv,
execArgv: debug ? devExecArgv : prodExecArgv,
},
};
}

function extensionVersionCompatibleWithAllProjects(serverModuleLocation: string): boolean {
const languageServiceVersion =
Expand Down
4 changes: 2 additions & 2 deletions server/src/banner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ export function requireOverride(moduleName: string) {
throw new Error(`Import '${TSSERVER}' instead of 'typescript'`);
}
if (moduleName === TSSERVER) {
const {tsProbeLocations} = parseCommandLine(process.argv);
moduleName = resolveTsServer(tsProbeLocations).resolvedPath;
const {tsProbeLocations, tsdk} = parseCommandLine(process.argv);
moduleName = resolveTsServer(tsProbeLocations, tsdk).resolvedPath;
}
return originalRequire(moduleName);
}
Expand Down
2 changes: 2 additions & 0 deletions server/src/cmdline_utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ interface CommandLineOptions {
logToConsole: boolean;
ngProbeLocations: string[];
tsProbeLocations: string[];
tsdk: string|null;
includeAutomaticOptionalChainCompletions: boolean;
includeCompletionsWithSnippetText: boolean;
forceStrictTemplates: boolean;
Expand All @@ -47,6 +48,7 @@ export function parseCommandLine(argv: string[]): CommandLineOptions {
logToConsole: hasArgument(argv, '--logToConsole'),
ngProbeLocations: parseStringArray(argv, '--ngProbeLocations'),
tsProbeLocations: parseStringArray(argv, '--tsProbeLocations'),
tsdk: findArgument(argv, '--tsdk') ?? null,
includeAutomaticOptionalChainCompletions:
hasArgument(argv, '--includeAutomaticOptionalChainCompletions'),
includeCompletionsWithSnippetText: hasArgument(argv, '--includeCompletionsWithSnippetText'),
Expand Down
6 changes: 4 additions & 2 deletions server/src/server.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
* found in the LICENSE file at https://angular.io/license
*/

import {version as tsServerVersion} from 'typescript/lib/tsserverlibrary';

import {generateHelpMessage, parseCommandLine} from './cmdline_utils';

// Parse command line arguments
Expand All @@ -27,7 +29,7 @@ function main() {
logVerbosity: options.logVerbosity,
});

const ts = resolveTsServer(options.tsProbeLocations);
const ts = resolveTsServer(options.tsProbeLocations, options.tsdk);
const ng = resolveNgLangSvc(options.ngProbeLocations);

const isG3 = ts.resolvedPath.includes('/google3/');
Expand All @@ -51,7 +53,7 @@ function main() {

// Log initialization info
session.info(`Angular language server process ID: ${process.pid}`);
session.info(`Using ${ts.name} v${ts.version} from ${ts.resolvedPath}`);
session.info(`Imported typescript/lib/tsserverlibrary is version ${tsServerVersion}.`);
session.info(`Using ${ng.name} v${ng.version} from ${ng.resolvedPath}`);
if (logger.loggingEnabled()) {
session.info(`Log file: ${logger.getLogFileName()}`);
Expand Down
4 changes: 2 additions & 2 deletions server/src/tests/version_provider_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe('Node Module Resolver', () => {
const probeLocations = [__dirname];

it('should be able to resolve tsserver', () => {
const result = resolveTsServer(probeLocations);
const result = resolveTsServer(probeLocations, null);
expect(result).toBeDefined();
expect(result.resolvedPath).toMatch(/typescript\/lib\/tsserverlibrary.js$/);
});
Expand All @@ -23,7 +23,7 @@ describe('Node Module Resolver', () => {
// Resolve relative to cwd.
const absPath = resolve('node_modules/typescript/lib');
expect(isAbsolute(absPath)).toBeTrue();
const result = resolveTsServer([absPath]);
const result = resolveTsServer(probeLocations, absPath);
expect(result.resolvedPath.endsWith('typescript/lib/tsserverlibrary.js')).toBeTrue();
});

Expand Down
7 changes: 3 additions & 4 deletions server/src/version_provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,9 @@ function resolveWithMinVersion(
* Resolve `typescript/lib/tsserverlibrary` from the given locations.
* @param probeLocations
*/
export function resolveTsServer(probeLocations: string[]): NodeModule {
if (probeLocations.length > 0) {
// The first probe location is `typescript.tsdk` if it is specified.
const resolvedFromTsdk = resolveTsServerFromTsdk(probeLocations[0], probeLocations.slice(1));
export function resolveTsServer(probeLocations: string[], tsdk: string|null): NodeModule {
if (tsdk !== null) {
const resolvedFromTsdk = resolveTsServerFromTsdk(tsdk, probeLocations);
const minVersion = new Version(MIN_TS_VERSION);
if (resolvedFromTsdk !== undefined) {
if (resolvedFromTsdk.version.greaterThanOrEqual(minVersion)) {
Expand Down

0 comments on commit 46b9a56

Please # to comment.