Skip to content

refactor(language-service): pull out interfaces on package toplevel - #40621

Closed
ayazhafiz wants to merge 2 commits into
angular:masterfrom
ayazhafiz:ls-unifyi
Closed

ayazhafiz wants to merge 2 commits into
angular:masterfrom
ayazhafiz:ls-unifyi

Conversation

@ayazhafiz

Copy link
Copy Markdown
Contributor

Two motivations behind this change:

  1. We would like to expose the types of the Language Service to external
    users (like the VSCode extension) via the npm package, on the top
    level of the package
  2. We would like the View Engine and Ivy LS to share a common interface
    (notably after the inclusion of getTcb, the Ivy LS upholds a
    strict superset of ts.LanguageService; previously both VE and Ivy
    LS were aligned on ts.LanguageService.)

To this end, this commit refactors the exports on the toplevel of the
language-service/ package to just be types common to both the VE and
Ivy language services. The VE and Ivy build targets then import and use
these types accordingly, and the expectation is that an external user
will just import the relevant typings from the toplevel package without
diving into either the VE or Ivy sources.

Follow up on #40607

Does this PR introduce a breaking change?

  • Yes

Yes, the public API has changed. However I am not aware of any package using @angular/language-service for its surface APIs other than the VSCode extension, for which this change is motivated by.

@ayazhafiz
ayazhafiz requested a review from kyliau January 28, 2021 20:51
@google-cla google-cla Bot added the cla: yes label Jan 28, 2021
@pullapprove
pullapprove Bot requested a review from josephperrott January 28, 2021 20:51
Comment thread packages/language-service/BUILD.bazel Outdated
Comment thread packages/language-service/ivy/language_service.ts Outdated
Comment thread packages/language-service/language-service.ts Outdated
Comment thread packages/language-service/ivy/ts_plugin.ts Outdated
Comment thread packages/language-service/src/ts_plugin.ts Outdated
@kyliau kyliau added the area: language-service Issues related to Angular's VS Code language service label Jan 28, 2021
@ngbot ngbot Bot added this to the Backlog milestone Jan 28, 2021
@ayazhafiz
ayazhafiz requested a review from kyliau January 29, 2021 01:41
Comment thread packages/language-service/api.ts Outdated
@kyliau

kyliau commented Feb 2, 2021

Copy link
Copy Markdown
Contributor

@ayazhafiz do you mind if i take this on so that we can get this merged before the release tomorrow?

@ayazhafiz

ayazhafiz commented Feb 2, 2021 via email

Copy link
Copy Markdown
Contributor Author

@kyliau
kyliau force-pushed the ls-unifyi branch 5 times, most recently from cd44fc8 to 81c1fd1 Compare February 3, 2021 00:25
@kyliau
kyliau requested review from gkalpak and removed request for josephperrott February 3, 2021 00:30
@kyliau kyliau added the target: patch This PR is targeted for the next patch release label Feb 3, 2021
Two motivations behind this change:

1. We would like to expose the types of the Language Service to external
   users (like the VSCode extension) via the npm package, on the top
   level of the package
2. We would like the View Engine and Ivy LS to share a common interface
   (notably after the inclusion of `getTcb`, the Ivy LS upholds a
   strict superset of `ts.LanguageService`; previously both VE and Ivy
   LS were aligned on `ts.LanguageService`.)

To this end, this commit refactors the exports on the toplevel of the
`language-service/` package to just be types common to both the VE and
Ivy language services. The VE and Ivy build targets then import and use
these types accordingly, and the expectation is that an external user
will just import the relevant typings from the toplevel package without
diving into either the VE or Ivy sources.

Follow up on angular#40607
@google-cla

This comment has been minimized.

@google-cla google-cla Bot added cla: no and removed cla: yes labels Feb 3, 2021
@gkalpak

This comment has been minimized.

@google-cla google-cla Bot added cla: yes and removed cla: no labels Feb 3, 2021
@gkalpak

gkalpak commented Feb 3, 2021 •

Copy link
Copy Markdown
Member

FYI, there was a lint failure due to the packages/language-service/BUILD.bazel file not being formatted according to buildifier's liking.

It turns out the problem was that glob() patterns should not contain constant strings. I pushed a fixup commit to fix it: 6882421

For reference, the error message from buildifier looked like this:

{
  "success": false,
  "files": [
    {
      "filename": "packages/language-service/BUILD.bazel",
      "formatted": true,
      "valid": true,
      "warnings": [
        {
          "start": {
            "line": 19,
            "column": 13
          },
          "end": {
            "line": 19,
            "column": 23
          },
          "category": "constant-glob",
          "actionable": true,
          "message": "Glob pattern \"index.ts\" has no wildcard ('*'). Constant patterns can be error-prone, move the file outside the glob.",
          "url": "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/bazelbuild/buildtools/blob/master/WARNINGS.md#constant-glob"
        }
      ]
    }
  ]
}

UPDATE: I have created angular/dev-infra#2346 to track the issue.

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM for dev-infra.

Reviewed-for: dev-infra

@pullapprove
pullapprove Bot requested a review from AndrewKushnir February 3, 2021 13:48
@kyliau
kyliau requested review from atscott and removed request for AndrewKushnir February 3, 2021 16:57
@kyliau kyliau added the action: merge The PR is ready for merge by the caretaker label Feb 3, 2021
@mhevery mhevery added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Feb 3, 2021
@mhevery mhevery closed this in 950875c Feb 3, 2021
kyliau pushed a commit to kyliau/angular that referenced this pull request Feb 3, 2021
…ngular#40621)

This is patch PR for angular#40621.

Two motivations behind this change:

1. We would like to expose the types of the Language Service to external
   users (like the VSCode extension) via the npm package, on the top
   level of the package
2. We would like the View Engine and Ivy LS to share a common interface
   (notably after the inclusion of `getTcb`, the Ivy LS upholds a
   strict superset of `ts.LanguageService`; previously both VE and Ivy
   LS were aligned on `ts.LanguageService`.)

To this end, this commit refactors the exports on the toplevel of the
`language-service/` package to just be types common to both the VE and
Ivy language services. The VE and Ivy build targets then import and use
these types accordingly, and the expectation is that an external user
will just import the relevant typings from the toplevel package without
diving into either the VE or Ivy sources.

Follow up on angular#40607

PR Close angular#40621
mhevery pushed a commit that referenced this pull request Feb 3, 2021
…40621) (#40695)

This is patch PR for #40621.

Two motivations behind this change:

1. We would like to expose the types of the Language Service to external
   users (like the VSCode extension) via the npm package, on the top
   level of the package
2. We would like the View Engine and Ivy LS to share a common interface
   (notably after the inclusion of `getTcb`, the Ivy LS upholds a
   strict superset of `ts.LanguageService`; previously both VE and Ivy
   LS were aligned on `ts.LanguageService`.)

To this end, this commit refactors the exports on the toplevel of the
`language-service/` package to just be types common to both the VE and
Ivy language services. The VE and Ivy build targets then import and use
these types accordingly, and the expectation is that an external user
will just import the relevant typings from the toplevel package without
diving into either the VE or Ivy sources.

Follow up on #40607

PR Close #40621

PR Close #40695
kyliau pushed a commit to kyliau/angular that referenced this pull request Feb 10, 2021
This is patch PR for angular#40621.

Two motivations behind this change:

1. We would like to expose the types of the Language Service to external
   users (like the VSCode extension) via the npm package, on the top
   level of the package
2. We would like the View Engine and Ivy LS to share a common interface
   (notably after the inclusion of `getTcb`, the Ivy LS upholds a
   strict superset of `ts.LanguageService`; previously both VE and Ivy
   LS were aligned on `ts.LanguageService`.)

To this end, this commit refactors the exports on the toplevel of the
`language-service/` package to just be types common to both the VE and
Ivy language services. The VE and Ivy build targets then import and use
these types accordingly, and the expectation is that an external user
will just import the relevant typings from the toplevel package without
diving into either the VE or Ivy sources.

Follow up on angular#40607
alxhub pushed a commit that referenced this pull request Feb 10, 2021
…40773)

This is patch PR for #40621.

Two motivations behind this change:

1. We would like to expose the types of the Language Service to external
   users (like the VSCode extension) via the npm package, on the top
   level of the package
2. We would like the View Engine and Ivy LS to share a common interface
   (notably after the inclusion of `getTcb`, the Ivy LS upholds a
   strict superset of `ts.LanguageService`; previously both VE and Ivy
   LS were aligned on `ts.LanguageService`.)

To this end, this commit refactors the exports on the toplevel of the
`language-service/` package to just be types common to both the VE and
Ivy language services. The VE and Ivy build targets then import and use
these types accordingly, and the expectation is that an external user
will just import the relevant typings from the toplevel package without
diving into either the VE or Ivy sources.

Follow up on #40607

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

Copy link
Copy Markdown

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 Mar 6, 2021
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: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants