Skip to content
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

Jump to bottom

feat(language-service): merge compiler options, watch extended configs #39134

Closed
wants to merge 1 commit into from
Closed

feat(language-service): merge compiler options, watch extended configs #39134

wants to merge 1 commit into from

Conversation

ayazhafiz
Copy link
Member

Currently the language service watches only the root config file of a
configured project for changes. However, since the Angular CLI merges
compiler options across all referenced config files, the Angular LS
should do the same, and watch for changes in any relevant config file.

This commit does the following:

  • Defer compiler option parsing to
    perform_compile.readConfiguration, which already merges compiler
    options across extended config files. Furthermore, return all
    extended config files from readConfiguration.
  • Attach file watchers to the project root config file, and all known
    extended config files as determined by readConfiguration. On a
    change to any config file, refresh the compiler options known by
    the Angular LS, and attach file watchers for any newly-extended
    config files.
    • Also: properly clean up file watchers, closing them when a file
      is deleted.

To test these changes, the mock server host is extended to support a
mock filesystem. This is needed because ngtsc reads files from a file
system interface, which the mock service does not provide. Furthermore,
methods on the mock filesystem defer to the mock host, so the host was
chosen to implement a filesystem rather than the service doing so.

@ayazhafiz ayazhafiz added feature Issue that requests a new feature area: language-service Issues related to Angular's VS Code language service target: patch This PR is targeted for the next patch release labels Oct 6, 2020
@ayazhafiz ayazhafiz requested a review from kyliau October 6, 2020 15:15
@ngbot ngbot bot added this to the needsTriage milestone Oct 6, 2020
@ayazhafiz
feat(language-service): merge compiler options, watch extended configs
4134ea6 
Currently the language service watches only the root config file of a
configured project for changes. However, since the Angular CLI merges
compiler options across all referenced config files, the Angular LS
should do the same, and watch for changes in any relevant config file.

This commit does the following:
   - Defer compiler option parsing to
     `perform_compile.readConfiguration`, which already merges compiler
     options across extended config files. Furthermore, return all
     extended config files from `readConfiguration`.
   - Attach file watchers to the project root config file, and all known
     extended config files as determined by `readConfiguration`. On a
     change to any config file, refresh the compiler options known by
     the Angular LS, and attach file watchers for any newly-extended
     config files.
     - Also: properly clean up file watchers, closing them when a file
       is deleted.

To test these changes, the mock server host is extended to support a
mock filesystem. This is needed because ngtsc reads files from a file
system interface, which the mock service does not provide. Furthermore,
methods on the mock filesystem defer to the mock host, so the host was
chosen to implement a filesystem rather than the service doing so.
ayazhafiz
ayazhafiz commented Oct 6, 2020
View reviewed changes
packages/language-service/ivy/test/mock_host.ts
Comment on lines +64 to +69
* File system mocking is needed to test dynamic changes of non-script files (namely, config files)
* that are watched and read from the file system rather than from a project service.
* NB: If you need to modify TS and template file contents, use the methods
* provided the `MockService` rather than this mock filesystem.
*/
export class MockHost extends NodeJSFileSystem implements ts.server.ServerHost {
Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to merge the mock filesystem with the mocking that happens in the MockService, lmk if you have any ideas. Otherwise this can be a follow-up cleanup after some more thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that the mock file system/mock service rely on the host, but the host also relies on the mock FS to watch files, so if we pull the mock FS down to the mock service then we required shared state b/w the host and the mock service/fs.

@AndrewKushnir AndrewKushnir requested review from alxhub and removed request for AndrewKushnir October 6, 2020 16:48
@ayazhafiz ayazhafiz moved this from Done to PRs In Review in Ivy Language Service Oct 6, 2020
@ayazhafiz ayazhafiz marked this pull request as draft October 8, 2020 22:09
@ayazhafiz ayazhafiz mentioned this pull request Oct 18, 2020
Closed
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Oct 18, 2020
@ayazhafiz
refactor(compiler-cli): accept a host for reading project configuration
65bbe75 
Currently the `readConfiguration` function in the compiler relies on the
file system to perform disk utilities needed to read determine a project
configuration file and read it. This poses a challenge for the language
service, which would like to use `readConfiguration` to watch and read
configurations dependent on extended tsconfigs (angular#39134). Challenges are
at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

As discussed in the LS sync, we decided to provide to
`readConfiguration` a host that would take of reading configuration as
needed for the use case of the caller. A first attempt was to put this
on `NgCompilerAdapter`, but this is not elegant because it introduces a
`CompilerAdatpter` dependency where there is no need for one (e.g. when
creating a watch mode host).

To be honest, I am not in love with the `ReadConfigurationHost`
delivered in this patch. I think it may be less hassle in the long term
and more ergonomic to create a `FileSystem` abstraction on a per-project
basis, delivered to the compiler whenever a new langauge service is
created. Let me know what you think.

Unblocks angular#39134
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Oct 18, 2020
@ayazhafiz
refactor(compiler-cli): accept a host for reading project configuration
fa54b5c 
Currently the `readConfiguration` function in the compiler relies on the
file system to perform disk utilities needed to read determine a project
configuration file and read it. This poses a challenge for the language
service, which would like to use `readConfiguration` to watch and read
configurations dependent on extended tsconfigs (angular#39134). Challenges are
at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

As discussed in the LS sync, we decided to provide to
`readConfiguration` a host that would take of reading configuration as
needed for the use case of the caller. A first attempt was to put this
on `NgCompilerAdapter`, but this is not elegant because it introduces a
`CompilerAdatpter` dependency where there is no need for one (e.g. when
creating a watch mode host).

To be honest, I am not in love with the `ReadConfigurationHost`
delivered in this patch. I think it may be less hassle in the long term
and more ergonomic to create a `FileSystem` abstraction on a per-project
basis, delivered to the compiler whenever a new langauge service is
created. Let me know what you think.

Unblocks angular#39134
@ayazhafiz ayazhafiz mentioned this pull request Oct 18, 2020
Closed
14 tasks
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Oct 19, 2020
@ayazhafiz
refactor(compiler-cli): accept a host for reading project configuration
37d2e4b 
Currently the `readConfiguration` function in the compiler relies on the
file system to perform disk utilities needed to read determine a project
configuration file and read it. This poses a challenge for the language
service, which would like to use `readConfiguration` to watch and read
configurations dependent on extended tsconfigs (angular#39134). Challenges are
at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

As discussed in the LS sync, we decided to provide to
`readConfiguration` a host that would take of reading configuration as
needed for the use case of the caller. A first attempt was to put this
on `NgCompilerAdapter`, but this is not elegant because it introduces a
`CompilerAdatpter` dependency where there is no need for one (e.g. when
creating a watch mode host).

To be honest, I am not in love with the `ReadConfigurationHost`
delivered in this patch. I think it may be less hassle in the long term
and more ergonomic to create a `FileSystem` abstraction on a per-project
basis, delivered to the compiler whenever a new langauge service is
created. Let me know what you think.

Unblocks angular#39134
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Oct 30, 2020
@ayazhafiz
feat(language-service): expose project-based FileSystem to compiler
32010cb 
Currently the `readConfiguration` function in the compiler relies on the
file system to perform disk utilities needed to read determine a project
configuration file and read it. This poses a challenge for the language
service, which would like to use `readConfiguration` to watch and read
configurations dependent on extended tsconfigs (angular#39134). Challenges are
at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

This patch alleviates the concern by directly providing to the compiler
a file system derived from the project owned by the language service.
Furthermore, the langauge service file system is "specialized" in some
sense - for one, it avoids implementing APIs that would change the state
of the underlying system, as no path in the language service
(server-side) should do this.
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Nov 3, 2020
@ayazhafiz
feat(language-service): expose project-based FileSystem to compiler
f2c5622 
Currently the `readConfiguration` function in the compiler relies on the
file system to perform disk utilities needed to read determine a project
configuration file and read it. This poses a challenge for the language
service, which would like to use `readConfiguration` to watch and read
configurations dependent on extended tsconfigs (angular#39134). Challenges are
at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

This patch alleviates the concern by directly providing to the compiler
a file system derived from the project owned by the language service.
Furthermore, the langauge service file system is "specialized" in some
sense - for one, it avoids implementing APIs that would change the state
of the underlying system, as no path in the language service
(server-side) should do this.
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Nov 10, 2020
@ayazhafiz
refactor(compiler-cli): provide a host to readConfiguration
89d8a97 
Currently `readConfiguration` relies on the file system to perform disk
utilities needed to read determine a project configuration file and read
it. This poses a challenge for the language service, which would like to
use `readConfiguration` to watch and read configurations dependent on
extended tsconfigs (angular#39134). Challenges are at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

This patch alleviates the concern by directly providing to the compiler
a "ParseConfigurationHost" with read-only "file system"-like utilties.
For the language service, this host is derived from the project owned by
the language service.

For more discussion see
https://docs.google.com/document/d/1TrbT-m7bqyYZICmZYHjnJ7NG9Vzt5Rd967h43Qx8jw0/edit?usp=sharing
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Nov 16, 2020
@ayazhafiz
refactor(compiler-cli): provide a host to readConfiguration
953fa0a 
Currently `readConfiguration` relies on the file system to perform disk
utilities needed to read determine a project configuration file and read
it. This poses a challenge for the language service, which would like to
use `readConfiguration` to watch and read configurations dependent on
extended tsconfigs (angular#39134). Challenges are at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

This patch alleviates the concern by directly providing to the compiler
a "ParseConfigurationHost" with read-only "file system"-like utilties.
For the language service, this host is derived from the project owned by
the language service.

For more discussion see
https://docs.google.com/document/d/1TrbT-m7bqyYZICmZYHjnJ7NG9Vzt5Rd967h43Qx8jw0/edit?usp=sharing
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Nov 17, 2020
@ayazhafiz
refactor(compiler-cli): provide a host to readConfiguration
0a4c40e 
Currently `readConfiguration` relies on the file system to perform disk
utilities needed to read determine a project configuration file and read
it. This poses a challenge for the language service, which would like to
use `readConfiguration` to watch and read configurations dependent on
extended tsconfigs (angular#39134). Challenges are at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

This patch alleviates the concern by directly providing to the compiler
a "ParseConfigurationHost" with read-only "file system"-like utilties.
For the language service, this host is derived from the project owned by
the language service.

For more discussion see
https://docs.google.com/document/d/1TrbT-m7bqyYZICmZYHjnJ7NG9Vzt5Rd967h43Qx8jw0/edit?usp=sharing
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Nov 17, 2020
@ayazhafiz
refactor(compiler-cli): provide a host to readConfiguration
64d1809 
Currently `readConfiguration` relies on the file system to perform disk
utilities needed to read determine a project configuration file and read
it. This poses a challenge for the language service, which would like to
use `readConfiguration` to watch and read configurations dependent on
extended tsconfigs (angular#39134). Challenges are at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

This patch alleviates the concern by directly providing to the compiler
a "ParseConfigurationHost" with read-only "file system"-like utilties.
For the language service, this host is derived from the project owned by
the language service.

For more discussion see
https://docs.google.com/document/d/1TrbT-m7bqyYZICmZYHjnJ7NG9Vzt5Rd967h43Qx8jw0/edit?usp=sharing
atscott pushed a commit that referenced this pull request Nov 17, 2020
@ayazhafiz @atscott
refactor(compiler-cli): provide a host to readConfiguration (#39619)
64c3135 
Currently `readConfiguration` relies on the file system to perform disk
utilities needed to read determine a project configuration file and read
it. This poses a challenge for the language service, which would like to
use `readConfiguration` to watch and read configurations dependent on
extended tsconfigs (#39134). Challenges are at least twofold:

1. To test this, the langauge service would need to provide to the
   compiler a mock file system.
2. The language service uses file system utilities primarily through
   TypeScript's `Project` abstraction. In general this should correspond
   to the underlying file system, but it may differ and it is better to
   go through one channel when possible.

This patch alleviates the concern by directly providing to the compiler
a "ParseConfigurationHost" with read-only "file system"-like utilties.
For the language service, this host is derived from the project owned by
the language service.

For more discussion see
https://docs.google.com/document/d/1TrbT-m7bqyYZICmZYHjnJ7NG9Vzt5Rd967h43Qx8jw0/edit?usp=sharing

PR Close #39619
@ayazhafiz ayazhafiz closed this Dec 4, 2020
@alxhub alxhub moved this from PRs In Review to Done in Ivy Language Service Dec 8, 2020
@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 Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kyliau kyliau Awaiting requested review from kyliau

Assignees
No one assigned
Labels
area: language-service Issues related to Angular's VS Code language service cla: yes feature Issue that requests a new feature target: patch This PR is targeted for the next patch release
Projects
No open projects
Ivy Language Service
  
Done
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ayazhafiz @googlebot