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

ivanwonder · 2025-05-09T03:26:43Z

…t about the angular metadata.

Enable the developer to disable the auto-import for the external module. If disabled, the completion only includes the info from the file scope and the ng-module scope.

Fixes #2132

ivanwonder · 2025-05-09T03:45:18Z

Blocked by angular/angular#61122

ivanwonder · 2025-06-05T02:23:58Z

This should be merged before the release of version 20.1.0. @atscott

atscott · 2025-06-05T20:26:42Z

package.json

+        "angular.suggest.autoImports": {
+          "type": "boolean",
+          "default": true,
+          "markdownDescription": "Enable/disable auto import suggestions."
+        },


I think the name of this option is a bit misleading? If I understand correctly, it doesn't enable/disable all auto imports, just the "global" completions. Can you update this description and option name?

Yes, it confuses me too. But this description is from the typescript. So I just copied it.

https://github.com/microsoft/vscode/blob/998fb692680cc20d70a805df8fa59cb1443bc897/extensions/typescript-language-features/package.nls.json#L90
https://github.com/microsoft/vscode/blob/998fb692680cc20d70a805df8fa59cb1443bc897/extensions/typescript-language-features/src/languageFeatures/fileConfigurationManager.ts#L208

😅 well we can do better than that unless I’m missing something. Maybe the option in typescript evolved over time and used to mean what it says. Or maybe it was always just totally wrong 🤷‍♂️

Done. You can check the description again.

server/package.json

atscott · 2025-06-05T22:02:33Z

server/src/session.ts

    this.logger = options.logger;
    this.logToConsole = options.logToConsole;
+    defaultPreferences = {
+      ...defaultPreferences,
+      includeCompletionsForModuleExports: options.includeCompletionsForModuleExports,


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?

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.

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?

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

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.

“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.

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.

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.

atscott · 2025-06-05T22:03:18Z

I believe you can mark #2132 as fixed by this PR?

ivanwonder · 2025-06-06T16:26:42Z

server/src/tests/cmdline_utils_spec.ts

+  it('should parse without "includeCompletionsForModuleExports"', () => {
+    const options = parseCommandLine(['--tsProbeLocations', '/baz,/qux']);
+    expect(options.includeCompletionsForModuleExports).toEqual(true);
+  });


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

ivanwonder · 2025-06-07T03:49:45Z

integration/lsp/BUILD.bazel

+        "//integration/project:node_modules/@angular/core",
+        "//integration/project:node_modules/@angular/common",


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

@atscott It's ready for review.

CI is broken unfortunately. Did you mean to include the test in this PR along with the addition of these deps?

I run the ivy_spec.ts on my computer, and there are some failures that are not related to my change. I found it because the project has not installed @angular/core and @angular/common, so I added it here.

consistent with the pre_standalone_project.

There is one expected error here: the auto-import from a different file. After setting the default value for the new configuration to true, there are no errors in the ivy_spec.ts.

Ah, got it. Thanks for fixing that!

…t about the angular metadata. Enable the developer to disable the auto-import for the external module. If disabled, the completion only includes the info from the file scope and the ng-module scope. Fixes angular#2132

angular-robot bot added the detected: feature label May 9, 2025

ivanwonder force-pushed the ts-global-completions branch from db0644b to 26346e5 Compare May 9, 2025 03:31

ivanwonder force-pushed the ts-global-completions branch 2 times, most recently from 3325456 to 9616ece Compare June 5, 2025 02:05

atscott requested changes Jun 5, 2025

View reviewed changes

atscott reviewed Jun 5, 2025

View reviewed changes

server/package.json Outdated Show resolved Hide resolved

atscott reviewed Jun 5, 2025

View reviewed changes

ivanwonder force-pushed the ts-global-completions branch 4 times, most recently from f9e2987 to 06f4f92 Compare June 6, 2025 13:03

ivanwonder requested a review from atscott June 6, 2025 13:28

ivanwonder commented Jun 6, 2025

View reviewed changes

ivanwonder marked this pull request as ready for review June 7, 2025 03:45

ivanwonder commented Jun 7, 2025

View reviewed changes

ivanwonder force-pushed the ts-global-completions branch from c61de8f to d627c08 Compare June 7, 2025 12:44

atscott approved these changes Jun 10, 2025

View reviewed changes

atscott added target: minor This PR is targeted for the next minor release action: merge Ready to merge labels Jun 10, 2025

atscott merged commit a39e01d into angular:main Jun 10, 2025
4 of 5 checks passed

	includeCompletionsForModuleExports: options.includeCompletionsForModuleExports,
	includeCompletionsForModuleExports: options.includeCompletionsForModuleExports ?? true,

		"//integration/project:node_modules/@angular/core",
		"//integration/project:node_modules/@angular/common",

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

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

Uh oh!

Conversation

ivanwonder commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanwonder commented May 9, 2025

Uh oh!

ivanwonder commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atscott commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ivanwonder commented May 9, 2025 •

edited

Loading