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.

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

fix(language-service): Return empty external files during project initialization #32519

Closed
wants to merge 1 commit into from

Conversation

@kyliau
Copy link
Member

commented Sep 6, 2019

This PR partially fixes a circular dependency problem whereby the
creation of a project queries Angular plugin for external files, but
the discovery of external files requires root files to be defined in a
Project. The right approach is to return empty array if Project has no
root files.

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 requested a review from ayazhafiz Sep 6, 2019
@kyliau kyliau requested a review from angular/tools-language-service as a code owner Sep 6, 2019
@ngbot ngbot bot added this to the needsTriage milestone Sep 6, 2019
@googlebot googlebot added the cla: yes label Sep 6, 2019
@kyliau kyliau force-pushed the kyliau:external_files branch from 3a1c16a to 536415f Sep 6, 2019
@kyliau kyliau added the PR state: WIP label Sep 6, 2019
@@ -6,23 +6,45 @@
* found in the LICENSE file at https://angular.io/license
*/

import * as tss from 'typescript/lib/tsserverlibrary'; // used as type only
import * as ts from 'typescript/lib/tsserverlibrary'; // used as value

This comment has been minimized.

Copy link
@ayazhafiz

ayazhafiz Sep 6, 2019

Contributor

Why can tsserverlibrary be used as a value now? If it is still the case that we can only use certain things as types, we should consider adding a document listing them later.

This comment has been minimized.

Copy link
@kyliau

kyliau Sep 6, 2019

Author Member

No, it cannot, thus the tests are all failing. I'll have to spend some time to think about this ...

This comment has been minimized.

Copy link
@kyliau

kyliau Sep 8, 2019

Author Member

Since the value provided at runtime is really typescript/lib/tsserverlibrary and tsserverlibrary is a superset of typescript, the import of both modules is replaced by the value provided at runtime in the rollup shim. This reduces the mental overhead when importing typescript in language service - there is no more distinction between import for types vs import for value.

@kyliau kyliau force-pushed the kyliau:external_files branch from 536415f to b02faf9 Sep 8, 2019
…tialization

This PR partially fixes a circular dependency problem whereby the
creation of a project queries Angular plugin for external files, but
the discovery of external files requires root files to be defined in a
Project. The right approach is to return empty array if Project has no
root files.
@kyliau kyliau force-pushed the kyliau:external_files branch from b02faf9 to 3f51164 Sep 8, 2019
@kyliau

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

@ayazhafiz I think this is good now, PTAL. Please let me know if there's any clarification needed in the documentation.

@kyliau kyliau removed the PR state: WIP label Sep 8, 2019
Copy link
Contributor

left a comment

👍 nice! I guess this means we can remove all type/value import comments in the LS.

@matsko matsko closed this in a65d3fa Sep 9, 2019
@kyliau kyliau deleted the kyliau:external_files branch Sep 9, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…tialization (angular#32519)

This PR partially fixes a circular dependency problem whereby the
creation of a project queries Angular plugin for external files, but
the discovery of external files requires root files to be defined in a
Project. The right approach is to return empty array if Project has no
root files.

PR Close angular#32519
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 10, 2019

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 Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.