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(ivy): incorrectly remapping certain properties that refer to inputs #28765

Closed
wants to merge 2 commits into from

Conversation

crisbeto
Copy link
Member

During build time we remap particular property bindings, because their names don't match their attribute equivalents (e.g. the property for the for attribute is called htmlFor). This breaks down if the particular element has an input that has the same name, because the property gets mapped to something invalid.

The following changes address the issue by mapping the name during runtime, because that's when directives are resolved and we know all of the inputs that are associated with a particular element.

@crisbeto crisbeto self-assigned this Feb 15, 2019
@crisbeto crisbeto requested review from a team as code owners February 15, 2019 20:46
@crisbeto crisbeto changed the title fix(ivy): incorrectly remapping certain properties that refer inputs fix(ivy): incorrectly remapping certain properties that refer to inputs Feb 15, 2019
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Feb 15, 2019
@crisbeto crisbeto removed their assignment Feb 15, 2019
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Changes look good to me, just needs some test updates

packages/core/test/render3/properties_spec.ts Outdated Show resolved Hide resolved
@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews comp: ivy and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 19, 2019
@ngbot ngbot bot added this to the needsTriage milestone Feb 19, 2019
@kara kara added the target: major This PR is targeted for the next major release label Feb 19, 2019
@crisbeto crisbeto force-pushed the FW-1063/property-mapping branch 2 times, most recently from 0fad6b8 to 6ed7120 Compare February 19, 2019 19:56
@crisbeto
Copy link
Member Author

@kara I've switched the tests over to TestBed and added a new test for the code generation. Can you take another look? The CI failures seem to be unrelated.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 19, 2019
@crisbeto crisbeto removed their assignment Feb 19, 2019
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

@crisbeto Just realized that this PR should fix a number of Material tests, but we're not actually removing anything from the blocklist. Can you remove the relevant tests?

https://github.com/angular/angular/blob/master/tools/material-ci/angular_material_test_blocklist.js#L1861

@crisbeto
Copy link
Member Author

Updated @kara. It also has two new bonus failures that are due to static queries.

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

LGTM for the changes in tools/material-ci/angular_material_test_blocklist.js (the ones I assumed triggered the angular/fw-dev-infra review).

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, though it needs a rebase

@crisbeto
Copy link
Member Author

It's rebased @kara.

@googlebot

This comment has been minimized.

@googlebot
Copy link

CLAs look good, thanks!

Googlers can find more info about SignCLA and this PR by following this link.

@kara kara removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 20, 2019
@kara kara unassigned mhevery and kara Feb 20, 2019
@kara kara added the action: presubmit The PR is in need of a google3 presubmit label Feb 20, 2019
@kara
Copy link
Contributor

kara commented Feb 20, 2019

presubmit

@kara kara added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker labels Feb 20, 2019
During build time we remap particular property bindings, because their names don't match their attribute equivalents (e.g. the property for the `for` attribute is called `htmlFor`). This breaks down if the particular element has an input that has the same name, because the property gets mapped to something invalid.

The following changes address the issue by mapping the name during runtime, because that's when directives are resolved and we know all of the inputs that are associated with a particular element.
@kara kara added the action: merge The PR is ready for merge by the caretaker label Feb 22, 2019
@IgorMinar IgorMinar closed this in 93a7836 Feb 22, 2019
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants