X Tutup
Skip to content

feat(compiler): detect dangling property bindings#2598

Merged
pkozlowski-opensource merged 1 commit intoangular:masterfrom
pkozlowski-opensource:danglig_prop_bindings
Jun 20, 2015
Merged

feat(compiler): detect dangling property bindings#2598
pkozlowski-opensource merged 1 commit intoangular:masterfrom
pkozlowski-opensource:danglig_prop_bindings

Conversation

@pkozlowski-opensource
Copy link
Copy Markdown
Member

@pkozlowski-opensource pkozlowski-opensource commented Jun 17, 2015

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
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 17, 2015

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
Copy link
Copy Markdown
Member Author

@pkozlowski-opensource pkozlowski-opensource Jun 18, 2015

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
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 18, 2015

@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 Outdated
Copy link
Copy Markdown
Contributor

@tbosch tbosch Jun 19, 2015

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
Copy Markdown
Member Author

@pkozlowski-opensource pkozlowski-opensource Jun 19, 2015

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
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 19, 2015

@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 Outdated
Copy link
Copy Markdown
Contributor

@mhevery mhevery Jun 19, 2015

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
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 20, 2015

@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
Copy Markdown

angular-automatic-lock-bot bot commented Sep 6, 2019

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
X Tutup