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.

Sign up for GitHub

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

Jump to bottom

feat(compiler): detect dangling property bindings #2598

Merged
merged 1 commit into from
Jun 20, 2015
Merged

feat(compiler): detect dangling property bindings #2598

merged 1 commit into from
Jun 20, 2015

Conversation

pkozlowski-opensource
Copy link
Member

BREAKING CHANGE: compiler will throw on binding to non-existing properties.

Till now it was possible to have a binding to a non-existing property,
ex.: <div [foo]="exp">. From now on this is compilation error - any
property binding needs to have at least one associated property:
eaither on an HTML element or on any directive associated with a
given element (directives' properites need to be declared using the
properties field in the @Directive / @Component annotation).

@pkozlowski-opensource
Copy link
Member Author

This is part of #2014 that takes only run-time generated schema into account. Opening this one asap to validate the approach (collecting property bindings and directive bindings in compile steps - we do it anyway - and do verifications in the ProtoViewBuilder). Adding schema from other sources (@View or global) should be trivial when this approach is validated)

@pkozlowski-opensource pkozlowski-opensource added this to the alpha-28 milestone Jun 17, 2015
@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 17, 2015
@pkozlowski-opensource pkozlowski-opensource force-pushed the danglig_prop_bindings branch 4 times, most recently from 58b91e5 to 01ffa28 Compare June 18, 2015 09:45
pkozlowski-opensource
pkozlowski-opensource reviewed Jun 18, 2015
View reviewed changes
modules/benchmarks/src/largetable/largetable_benchmark.ts
@@ -232,7 +232,7 @@ class CellData {
</tbody>
<tbody template="ng-switch-when 'interpolationAttr'">
<tr template="ng-for #row of data">
<td template="ng-for #column of row" i="{{column.i}}" j="{{column.j}}">
<td template="ng-for #column of row" attr.i="{{column.i}}" attr.j="{{column.j}}">
Copy link
Member Author

Choose a reason for hiding this comment

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

@tbosch I hope that this doesn't change the nature of this test too much - I had to modify this one as we don't support binding to non-existing properties after this change.

@pkozlowski-opensource
Copy link
Member Author

@mhevery this is first step of full schema support that detects bindings to non-existing properties. It would be great if this one could be reviewed / approach validated so I can move on.

@tbosch tbosch assigned tbosch and unassigned mhevery Jun 18, 2015
tbosch
tbosch reviewed Jun 19, 2015
View reviewed changes
modules/angular2/src/render/dom/compiler/directive_parser.ts
@@ -151,6 +151,7 @@ export class DirectiveParser implements CompileStep {
var fullExpAstWithBindPipes = this._parser.addPipes(bindingAst, pipes);
directiveBinderBuilder.bindProperty(dirProperty, fullExpAstWithBindPipes);
}
compileElement.bindElement().bindPropertyToDirective(dashCaseToCamelCase(elProp));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this extra information? The ProtoViewBuilder has all binding information of the element and all of it's directives already...

Copy link
Member Author

Choose a reason for hiding this comment

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

@tbosch I believe that we need it due to the properties re-mapping. When we bind to directives we know only about property name on the directive side, but not about the original property binding.

@pkozlowski-opensource
Copy link
Member Author

@tbosch I've rebased it, PTAL

@pkozlowski-opensource pkozlowski-opensource force-pushed the danglig_prop_bindings branch 3 times, most recently from b1a53ba to 69cbb0f Compare June 19, 2015 21:44
mhevery
mhevery reviewed Jun 19, 2015
View reviewed changes
modules/angular2/test/core/compiler/integration_spec.ts

PromiseWrapper.catchError(tb.createView(MyComp, {context: ctx}), (e) => {
expect(e.message).toEqual(
`Can't bind to 'unknown' since it isn't a know property of the 'div' element`);
Copy link
Contributor

Choose a reason for hiding this comment

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

change error to include:

Can't bind to 'unknown' since it isn't a know property of the 'div' element, and there are no directives present which can accept that value.

Or something like that.

@mhevery mhevery added pr_state: LGTM action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 19, 2015
@pkozlowski-opensource
feat(compiler): detect dangling property bindings
d7b9345 
BREAKING CHANGE: compiler will throw on binding to non-existing properties.

Till now it was possible to have a binding to a non-existing property,
ex.: `<div [foo]="exp">`. From now on this is compilation error - any
property binding needs to have at least one associated property:
eaither on an HTML element or on any directive associated with a
given element (directives' properites need to be declared using the
`properties` field in the `@Directive` / `@Component` annotation).

Closes angular#2598
@pkozlowski-opensource pkozlowski-opensource merged commit d7b9345 into angular:master Jun 20, 2015
@pkozlowski-opensource
Copy link
Member Author

@mhevery thnx for the review. I've changed the error message and merged to unblock @tbosch changes. Will add tests indicated in the review in a subsequent commit.

@pkozlowski-opensource pkozlowski-opensource mentioned this pull request Jun 21, 2015
Closed
@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@pkozlowski-opensource pkozlowski-opensource

Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker cla: yes
Projects
None yet
Milestone
alpha-28
Development

Successfully merging this pull request may close these issues.
4 participants
@pkozlowski-opensource @mhevery @tbosch @googlebot