Skip to content

feat(language-service): Support importing the external module's expor… #2173

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

Open
wants to merge 3 commits into
base: main
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
.npmrc=974837034
pnpm-lock.yaml=1493175183
yarn.lock=485928896
package.json=-1370450906
package.json=-100959756
pnpm-workspace.yaml=1711114604
7 changes: 7 additions & 0 deletions client/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,13 @@ export class AngularLanguageClient implements vscode.Disposable {
args.push('--includeCompletionsWithSnippetText');
}

const includeCompletionsForModuleExports = config.get<boolean>('angular.suggest.autoImports');
args.push(
'--includeCompletionsForModuleExports',
includeCompletionsForModuleExports === undefined ?
'true' :
includeCompletionsForModuleExports.toString());

// Sort the versions from oldest to newest.
const angularVersions = (await getAngularVersionsInWorkspace(this.outputChannel))
.sort((a, b) => a.version.greaterThanOrEqual(b.version) ? 1 : -1);
Expand Down
2 changes: 2 additions & 0 deletions integration/lsp/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ jasmine_test(
data = [
"//integration",
"//integration/project",
"//integration/project:node_modules/@angular/core",
"//integration/project:node_modules/@angular/common",
Comment on lines +9 to +10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The LSP test has passed. Currently, the CI error is due to a timeout issue.

@atscott It's ready for review.

"//integration/pre_standalone_project",
"//integration/pre_standalone_project:node_modules/@angular/core",
"//integration/pre_standalone_project:node_modules/@angular/common",
Expand Down
5 changes: 5 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@
"default": true,
"markdownDescription": "Enable snippet completions from Angular language server. Requires using TypeScript 4.3+ in the workspace."
},
"angular.suggest.autoImports": {
"type": "boolean",
"default": true,
"markdownDescription": "Enable/disable auto import suggestions for the exported Angular components from the current project."
},
"angular.forceStrictTemplates": {
"type": "boolean",
"default": false,
Expand Down
28 changes: 28 additions & 0 deletions server/src/cmdline_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,31 @@ function findArgument(argv: string[], argName: string): string|undefined {
return argv[index + 1];
}

function findArgumentWithDefault<T>(argv: string[], argName: string, defaultValue: T): T|string|
undefined {
const index = argv.indexOf(argName);
if (index < 0) {
return defaultValue;
}
if (index === argv.length - 1) {
return undefined;
}
const argValue = argv[index + 1];
if (argValue.startsWith('-')) {
return undefined;
} else {
return argValue;
}
}

function parseBooleanArgument(argv: string[], argName: string): boolean {
const argValue = findArgumentWithDefault(argv, argName, 'true');
if (argValue === undefined || argValue === 'true') {
return true;
}
return false;
}

function parseStringArray(argv: string[], argName: string): string[] {
const arg = findArgument(argv, argName);
if (!arg) {
Expand All @@ -36,6 +61,7 @@ interface CommandLineOptions {
tsdk: string|null;
includeAutomaticOptionalChainCompletions: boolean;
includeCompletionsWithSnippetText: boolean;
includeCompletionsForModuleExports: boolean;
forceStrictTemplates: boolean;
disableBlockSyntax: boolean;
disableLetSyntax: boolean;
Expand All @@ -55,6 +81,8 @@ export function parseCommandLine(argv: string[]): CommandLineOptions {
includeAutomaticOptionalChainCompletions:
hasArgument(argv, '--includeAutomaticOptionalChainCompletions'),
includeCompletionsWithSnippetText: hasArgument(argv, '--includeCompletionsWithSnippetText'),
includeCompletionsForModuleExports:
parseBooleanArgument(argv, '--includeCompletionsForModuleExports'),
forceStrictTemplates: hasArgument(argv, '--forceStrictTemplates'),
disableBlockSyntax: hasArgument(argv, '--disableBlockSyntax'),
disableLetSyntax: hasArgument(argv, '--disableLetSyntax'),
Expand Down
3 changes: 3 additions & 0 deletions server/src/completion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ export interface NgCompletionOriginData {

filePath: string;
position: lsp.Position;

tsData?: ts.CompletionEntryData;
}

/**
Expand Down Expand Up @@ -135,6 +137,7 @@ export function tsCompletionEntryToLspCompletionItem(
kind: 'ngCompletionOriginData',
filePath: scriptInfo.fileName,
position,
tsData: entry.data,
} as NgCompletionOriginData;
return item;
}
Expand Down
1 change: 1 addition & 0 deletions server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ function main() {
logToConsole: options.logToConsole,
includeAutomaticOptionalChainCompletions: options.includeAutomaticOptionalChainCompletions,
includeCompletionsWithSnippetText: options.includeCompletionsWithSnippetText,
includeCompletionsForModuleExports: options.includeCompletionsForModuleExports,
forceStrictTemplates: isG3 || options.forceStrictTemplates,
disableBlockSyntax: options.disableBlockSyntax,
disableLetSyntax: options.disableLetSyntax,
Expand Down
18 changes: 14 additions & 4 deletions server/src/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface SessionOptions {
logToConsole: boolean;
includeAutomaticOptionalChainCompletions: boolean;
includeCompletionsWithSnippetText: boolean;
includeCompletionsForModuleExports: boolean;
forceStrictTemplates: boolean;
disableBlockSyntax: boolean;
disableLetSyntax: boolean;
Expand All @@ -49,7 +50,7 @@ const EMPTY_RANGE = lsp.Range.create(0, 0, 0, 0);
const setImmediateP = promisify(setImmediate);

const defaultFormatOptions: ts.FormatCodeSettings = {};
const defaultPreferences: ts.UserPreferences = {};
let defaultPreferences: ts.UserPreferences = {};

const htmlLS = getHTMLLanguageService();

Expand All @@ -70,6 +71,7 @@ export class Session {
private readonly openFiles = new MruTracker();
private readonly includeAutomaticOptionalChainCompletions: boolean;
private readonly includeCompletionsWithSnippetText: boolean;
private readonly includeCompletionsForModuleExports: boolean;
private snippetSupport: boolean|undefined;
private diagnosticsTimeout: NodeJS.Timeout|null = null;
private isProjectLoading = false;
Expand All @@ -86,8 +88,13 @@ export class Session {
this.includeAutomaticOptionalChainCompletions =
options.includeAutomaticOptionalChainCompletions;
this.includeCompletionsWithSnippetText = options.includeCompletionsWithSnippetText;
this.includeCompletionsForModuleExports = options.includeCompletionsForModuleExports;
this.logger = options.logger;
this.logToConsole = options.logToConsole;
defaultPreferences = {
...defaultPreferences,
includeCompletionsForModuleExports: options.includeCompletionsForModuleExports,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
includeCompletionsForModuleExports: options.includeCompletionsForModuleExports,
includeCompletionsForModuleExports: options.includeCompletionsForModuleExports ?? true,

I think the absence of this option should then be true, right? I'm noticing that if I update the upstream @angular/language-service version without this PR, then I no longer get any completions for items not already in the component imports. So effectively, the auto import feature is entirely broken/turned off. I don't know if that was the intent, but at the very least, if it's supposed to default to true, then the default should also exist in the @angular/language-server so extensions for other IDEs don't regress in the ability to provide completions and automatic imports.

With the above, should the @angular/language-service fall back to the old behavior when this option is false or not present? Or use the old behavior if not present and the new behavior of only providing completions for items already in the imports if it's explicitly false?

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 don't know if that was the intent, but at the very least, if it's supposed to default to true, then the default should also exist in the @angular/language-server so extensions for other IDEs don't regress in the ability to provide completions and automatic imports.

I think the default value in the @angular/language-server can be true, because the exported API is defined by Angular. However, for the @angular/language-service, the LS exported API is defined by TypeScript. Should we change the definition of the config and overwrite the type?

I think the absence of this option should then be true, right? I'm noticing that if I update the upstream @angular/language-service version without this PR, then I no longer get any completions for items not already in the component imports.

Yes, for the developer who uses the @angular/language-service, only the imported component can be provided. The developer should pass the data and includeCompletionsForModuleExports support external module. Before the Angular LS ignores these configs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I think I understand. Though I don’t know about the part about Angular LS “ignoring these configs”. They didn’t exist on the Angular side before and if I understand correctly, they’re still configured separately so they still “ignore” the typescript option, right? Can we actually just use the typescript option instead of adding a matching one of our own where they can differ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the typescript language server implementation have a default value of true or is it always expected to be set by the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the typescript language server implementation have a default value of true or is it always expected to be set by the client?

This is why I need to set the includeCompletionsForModuleExports to true.

https://github.com/angular/angular/blob/8f9d13ef2bd4ddc10c230ddd0019ef21d9887175/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts#L921

“ignoring these configs”, I mean the Angular developer still can set includeCompletionsForModuleExports to false, but it doesn't work. for example,

ngLs.getCompletionsAtPosition(path, position, {
      includeCompletionsForModuleExports: false, // this doesn't work.
})

Or maybe I don't add this new config for now. Keep it the same as the old behavior, always provide the global component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

“ignoring these configs”, I mean the Angular developer still can set includeCompletionsForModuleExports to false, but it doesn't work. for example…

okay, right that makes sense. The option “existed” but wasn’t used.

practically speaking though, developers don’t really interact with the language service directly. It all goes through the language server. I don’t know of any integrations that implement their own server or use the language service directly, though there could be some out there.

I think we should consider changing the default here to true: https://github.com/angular/angular/blob/8f9d13ef2bd4ddc10c230ddd0019ef21d9887175/packages/language-service/src/completions.ts#L756

This might not match what typescript has for the default, but I think it could be considered a breaking change to the behavior from before the commit.

WDYT? I’m still wrapping my head around this so appreciate your patience walking through this with me.

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 think we should consider changing the default here to true: https://github.com/angular/angular/blob/8f9d13ef2bd4ddc10c230ddd0019ef21d9887175/packages/language-service/src/completions.ts#L756

The configuration is passed from the TypeScript language server to the language service. If the default value needs to be changed, two functions(getCompletionsAtPosition, getCompletionEntryDetails) must be updated.

For example, here needs to be updated too.
https://github.com/angular/angular/blob/8f9d13ef2bd4ddc10c230ddd0019ef21d9887175/packages/language-service/src/completions.ts#L847

practically speaking though, developers don’t really interact with the language service directly. It all goes through the language server. I don’t know of any integrations that implement their own server or use the language service directly, though there could be some out there.

If the developers don’t really interact with the language service directly. I think the language service doesn't need to be changed. Make sure no breaking changes are introduced in the language server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the developers don’t really interact with the language service directly. I think the language service doesn't need to be changed. Make sure no breaking changes are introduced in the language server.

Right, it probably doesn’t need to change if we change the default in the language server instead.

};
// Create a connection for the server. The connection uses Node's IPC as a transport.
this.connection = lsp.createConnection({
// cancelUndispatched is a "middleware" to handle all cancellation requests.
Expand Down Expand Up @@ -150,6 +157,7 @@ export class Session {
// We don't want the AutoImportProvider projects to be created. See
// https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/#smarter-auto-imports
includePackageJsonAutoImports: 'off',
includeCompletionsForModuleExports: this.includeCompletionsForModuleExports,
},
watchOptions: {
// Used as watch options when not specified by user's `tsconfig`.
Expand Down Expand Up @@ -1234,12 +1242,14 @@ export class Session {
let options: ts.GetCompletionsAtPositionOptions = {};
const includeCompletionsWithSnippetText =
this.includeCompletionsWithSnippetText && this.snippetSupport;
if (this.includeAutomaticOptionalChainCompletions || includeCompletionsWithSnippetText) {
if (this.includeAutomaticOptionalChainCompletions || includeCompletionsWithSnippetText ||
this.includeCompletionsForModuleExports) {
options = {
includeAutomaticOptionalChainCompletions: this.includeAutomaticOptionalChainCompletions,
includeCompletionsWithSnippetText: includeCompletionsWithSnippetText,
includeCompletionsWithInsertText:
this.includeAutomaticOptionalChainCompletions || includeCompletionsWithSnippetText,
includeCompletionsForModuleExports: this.includeCompletionsForModuleExports,
};
}

Expand Down Expand Up @@ -1269,8 +1279,8 @@ export class Session {

const offset = lspPositionToTsPosition(scriptInfo, position);
const details = languageService.getCompletionEntryDetails(
filePath, offset, item.insertText ?? item.label, undefined, undefined, undefined,
undefined);
filePath, offset, item.insertText ?? item.label, undefined, undefined, defaultPreferences,
data.tsData);
if (details === undefined) {
return item;
}
Expand Down
20 changes: 20 additions & 0 deletions server/src/tests/cmdline_utils_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,24 @@ describe('parseCommandLine', () => {
const options = parseCommandLine(['--tsProbeLocations', '/baz,/qux']);
expect(options.tsProbeLocations).toEqual(['/baz', '/qux']);
});

it('should parse without "includeCompletionsForModuleExports"', () => {
const options = parseCommandLine(['--tsProbeLocations', '/baz,/qux']);
expect(options.includeCompletionsForModuleExports).toEqual(true);
});
Comment on lines +37 to +40
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a test case when the config includeCompletionsForModuleExports doesn't exist.


it('should parse with "--includeCompletionsForModuleExports --help"', () => {
const options = parseCommandLine(['--includeCompletionsForModuleExports', '--help']);
expect(options.includeCompletionsForModuleExports).toEqual(true);
});

it('should parse with "--includeCompletionsForModuleExports true --help"', () => {
const options = parseCommandLine(['--includeCompletionsForModuleExports', 'true', '--help']);
expect(options.includeCompletionsForModuleExports).toEqual(true);
});

it('should parse with "--includeCompletionsForModuleExports false --help"', () => {
const options = parseCommandLine(['--includeCompletionsForModuleExports', 'false', '--help']);
expect(options.includeCompletionsForModuleExports).toEqual(false);
});
});
Loading