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

feat(common): better error message when non-template element used in NgIf #22274

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@trotyl
Contributor

trotyl commented Feb 17, 2018

closes #16410

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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: #16410

What is the new behavior?

When passing a non-template to NgIf, throwing an error rather than behaving incorrectly.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes label Feb 17, 2018

@@ -163,3 +165,14 @@ export class NgIfContext {
public $implicit: any = null;
public ngIf: any = null;
}
function assertTemplate(property: string, templateRef: TemplateRef<any>): void {
if (!templateRef.createEmbeddedView) {

This comment has been minimized.

@vicb

vicb Feb 17, 2018

Contributor

make the intent clearer please:

const isTemplateRef = templateRef.createEmbeddedView != null; 
if (!isTemplateRef) { ... }

This comment has been minimized.

@trotyl

trotyl Feb 17, 2018

Contributor

👍

}
}
function getTypeNameForDebugging(type: any): string {

This comment has been minimized.

@vicb

vicb Feb 17, 2018

Contributor

please use import {ɵstringify as stringify} from "@angular/core";

This comment has been minimized.

@trotyl

trotyl Feb 17, 2018

Contributor

Thanks, didn't know that before.

@vicb

vicb approved these changes Feb 17, 2018

some cleanup. Thanks for the PR !

@vicb

This comment has been minimized.

Contributor

vicb commented Feb 17, 2018

Please set the commit type to "feat". Thanks.

@trotyl

This comment has been minimized.

Contributor

trotyl commented Feb 17, 2018

@vicb The original issue is marked type: bug/fix, would that be inconsistent?

fixture = createTestComponent(template);
expect(() => fixture.detectChanges()).toThrow();

This comment has been minimized.

@vicb

vicb Feb 17, 2018

Contributor

sorry forgot to ask to check the message

This comment has been minimized.

@trotyl

trotyl Feb 17, 2018

Contributor

👍

@vicb

This comment has been minimized.

Contributor

vicb commented Feb 17, 2018

it's not a big deal if those don't match - and really we are not fixing smthg.
Thanks.

@trotyl trotyl changed the title from fix(common): better error message when non-template element used in NgIf to feat(common): better error message when non-template element used in NgIf Feb 17, 2018

@trotyl

This comment has been minimized.

Contributor

trotyl commented Feb 17, 2018

e2e failed due to jasmine timeout, should be flake. Request rerunning.

@vicb

vicb approved these changes Feb 17, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Feb 17, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "continuous-integration/travis-ci/pr" is failing
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@vicb

This comment has been minimized.

Contributor

vicb commented Feb 17, 2018

Thanks for the updates ! I have just restarted Travis.

cl/186104980

@trotyl

This comment has been minimized.

Contributor

trotyl commented Feb 17, 2018

Hope there's still CI quota today~ 😄

@vicb vicb closed this in 67cf11d Feb 18, 2018

jbogarthyde added a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment