-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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): use single entry point for Ivy and View Engine #40967
Conversation
packages/language-service/index.ts
Outdated
return plugin?.getExternalFiles?.(proj) || []; | ||
} | ||
|
||
export function onConfigurationChanged(config: any): void { | ||
plugin?.onConfigurationChanged?.(config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is a little bit awkward given that you have to call create
first but I can't think of a better way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether we can assert the plugin
has been created by tsserver. If not, the tsserver cannot invoke the function.
22b1ea7
to
dc71d5d
Compare
Quick question @kyliau - would this be considered a breaking change since it changes the "main" property in |
fd23451
to
e595b8d
Compare
@kyliau the entry_point is wrong for ve bundles.
|
e595b8d
to
2ef3142
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
(Just a couple of question - out of curiosity mostly 🧐)
Reviewed-for: dev-infra
Currently there are two entry points for the `@angular/language-service` package: - `@angular/language-service` This default entry point is for View Engine LS. Through the redirection of `main` field in `package.json`, it resolves to `./bundles/language-service.js`. - `@angular/language-service/bundles/ivy.js` This secondary entry point is for Ivy LS. TypeScript recently changed the behavior of tsserver to allow only package names as plugin names [1] for security reasons. This means the secondary entry point for Ivy LS can no longer be used. We implemented a quick hack in the module resolver (in the extension repo) to fix this, but the long term fix should be in `@angular/language-service`. Here, the `main` field in `package.json` is changed to `index.js`, and in the index file we conditionally load View Engine or Ivy based on the input config. This eliminates the need for multiple entry points. As part of this PR, I also removed all source code for View Engine and Ivy included in the NPM package. Consumers of this package should run the bundled output and nothing else. This would help us prevent an accidental import that results in execution of unbundled code. [1]: microsoft/TypeScript#42713
2ef3142
to
a2c9668
Compare
@atscott It's technically not a breaking change since |
@ivanwonder Thanks for pointing that out, I've fixed the entry point :) |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently there are two entry points for the
@angular/language-service
package:
@angular/language-service
This default entry point is for View Engine LS. Through the redirection
of
main
field inpackage.json
, it resolves to./bundles/language-service.js
.@angular/language-service/bundles/ivy.js
This secondary entry point is for Ivy LS.
TypeScript recently changed the behavior of tsserver to allow only package
names as plugin names 1 for security reasons. This means the secondary
entry point for Ivy LS can no longer be used.
We implemented a quick hack in the module resolver (in the extension repo)
to fix this, but the long term fix should be in
@angular/language-service
.Here, the
main
field inpackage.json
is changed toindex.js
, and in theindex file we conditionally load View Engine or Ivy based on the input config.
This eliminates the need for multiple entry points.
As part of this PR, I also removed all source code for View Engine and Ivy
included in the NPM package. Consumers of this package should run the bundled
output and nothing else. This would help us prevent an accidental import that
results in execution of unbundled code.
The new layout of the NPM package is as follows:
cc: @ivanwonder
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information