Skip to content

Conversation

kyliau
Copy link
Contributor

@kyliau kyliau commented May 4, 2020

Parse Angular compiler options in Angular language service.

In View Engine, only TypeScript compiler options are read, Angular
compiler options are not. With Ivy, there could be different modes of
compilation, most notably how strict the templates should be checked.
This commit makes the behavior of language service consistent with the
Ivy compiler.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@kyliau kyliau added area: language-service Issues related to Angular's VS Code language service target: major This PR is targeted for the next major release labels May 4, 2020
@kyliau kyliau requested a review from ayazhafiz May 4, 2020 21:21
@ngbot ngbot bot added this to the needsTriage milestone May 4, 2020
@pullapprove pullapprove bot requested a review from filipesilva May 4, 2020 21:21
Comment on lines +25 to +31
if (!(project instanceof ts.server.ConfiguredProject)) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if someone starts the LS in a project with no tsconfig, then adds a tsconfig later on? Not saying we should worry about it, but we won't detect the config change right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our application is using a prepare script to generate the tsconfig and angular configuration files because the files were getting huge, so we are now "building" the configuration files.

It would be nice to have this working with Ivy.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SchnWalter when is the config file built? Today TypeScript only supports static analysis of tsconfig.json files. The language service relies only on tsconfig, not on other config files like angular.json.

Copy link
Contributor

@SchnWalter SchnWalter May 4, 2020

Choose a reason for hiding this comment

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

We generate our tsconfig.json file on npm postinstall for CI pipelines and on localhost we also use some some IDE launch commands, and this allows us to test various combinations of libraries/projects. And it would be nice to not have to manually reload the TypeScript services or restart the IDEs whenever we regenerate the tsconfig files.

In the mono-repository we have quite a few libraries and several projects that consume them and most packages also support multiple "feature flags" which are also using various tsconfig path aliases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this kind of request is better targeted at TypeScript. Angular only consumes TypeScript APIs. Sounds like you are looking for a conditional build system that tsconfig does not support. There is a long-open proposal for conditional compilation directives, which could be somewhat related.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if someone starts the LS in a project with no tsconfig, then adds a tsconfig later on? Not saying we should worry about it, but we won't detect the config change right?

Good question! tsserver automatically takes care of situation like this. Without a tsconfig, an InferredProject is created to host the scripts. Upon creation of a tsconfig in the workspace, the scripts will be transferred to a ConfiguredProject, the old project will be disposed along with the language service, and a new language service will be created.
I added a TODO here to make sure we clean up the FileWatcher when this happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Parse Angular compiler options in Angular language service.

In View Engine, only TypeScript compiler options are read, Angular
compiler options are not. With Ivy, there could be different modes of
compilation, most notably how strict the templates should be checked.
This commit makes the behavior of language service consistent with the
Ivy compiler.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 5, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…#36922)

Parse Angular compiler options in Angular language service.

In View Engine, only TypeScript compiler options are read, Angular
compiler options are not. With Ivy, there could be different modes of
compilation, most notably how strict the templates should be checked.
This commit makes the behavior of language service consistent with the
Ivy compiler.

PR Close angular#36922
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: language-service Issues related to Angular's VS Code language service cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants