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): tolerate errors in decorators #14634

Merged
merged 1 commit into from
Mar 1, 2017

Conversation

chuckjaz
Copy link
Contributor

Fixes #14631

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix

What is the current behavior? (You can also link to an open issue here)

Metadata errors caused the language service to produce strange cascading errors causing problems such as reported here: angular/vscode-ng-language-service#10

What is the new behavior?

Metadata errors are reported and ignored.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

@@ -35,7 +35,7 @@ export class CompilerHost implements AotCompilerHost {

constructor(
protected program: ts.Program, protected options: AngularCompilerOptions,
protected context: CompilerHostContext) {
protected context: CompilerHostContext, collectorOptions?: CollectorOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be unused?

@@ -335,7 +343,8 @@ export class StaticReflector implements ReflectorReader {
if (value && (depth != 0 || value.__symbolic != 'error')) {
const parameters: string[] = targetFunction['parameters'];
const defaults: any[] = targetFunction.defaults;
args = args.map(arg => simplifyInContext(context, arg, depth + 1));
args = args.map(arg => simplifyInContext(context, arg, depth + 1))
.map(arg => shouldIgnore(arg) ? undefined : arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

could it be filter() instead? where do the undefined values go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No as that would change the argument order. Imagine the call a(1, () => {}, 2), I want the transformed into a(1, undefined, 2) not a(1,2).

@@ -18,6 +18,14 @@ const ANGULAR_CORE = '@angular/core';

const HIDDEN_KEY = /^\$.*\$$/;

const ignore = {
Copy link
Contributor

Choose a reason for hiding this comment

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

opt: all caps?

@@ -529,7 +538,8 @@ export class StaticReflector implements ReflectorReader {
let converter = self.conversionMap.get(staticSymbol);
if (converter) {
const args =
argExpressions.map(arg => simplifyInContext(context, arg, depth + 1));
argExpressions.map(arg => simplifyInContext(context, arg, depth + 1))
.map(arg => shouldIgnore(arg) ? undefined : arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above


}
`;
init(data, [], () => {}, {verboseInvalidExpression: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be a test for what happens with a Component with errors and verboseInvalidExpression is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in this case. There are tests elsewhere that test that. The only time we need to handle more than one error is the case where there is error reporter and verboseInvalidExpressions is true. Testing this with verboseInvalidExpression as false would simplify the expression to a single error node which is what the other tests are already testing.

@IgorMinar IgorMinar merged commit 6bae737 into angular:master Mar 1, 2017
SamVerschueren pushed a commit to SamVerschueren/angular that referenced this pull request Mar 18, 2017
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@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 Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StaticReflector elides annotations that contains errors
5 participants