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(upgrade): pass correct values to ngOnChanges for interpolation bindings #14301

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Feb 4, 2017

Please check if the PR fulfills these requirements

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

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
The previousValue and currentValue arguments passed to the SimpleChange constructor are swapped for interpolation bindings.
There are also some tests that may hide failures by allowing the $exceptionHandler to swallow thrown exceptions.

What is the new behavior?
The SimpleChange objects are created correctly.
Interpolation bindings and property bindings share the same implementation.
The tests no longer hide failures by swallowing exceptions.

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

[ ] Yes
[x] No

@gkalpak gkalpak added comp: upgrade/dynamic action: review The PR is still awaiting reviews from at least one requested reviewer type: bug/fix labels Feb 4, 2017
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -78,17 +78,15 @@ export class DowngradeComponentAdapter {
let expr: any /** TODO #9100 */ = null;

if (attrs.hasOwnProperty(input.attr)) {
const observeFn = ((prop: any /** TODO #9100 */) => {
const observeFn = (prop => {
Copy link
Member

Choose a reason for hiding this comment

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

Does the linter let you get away with an implicit any here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It infers it is a string (because input.prop is a string). No idea if/why it didn't before.

Copy link
Member

Choose a reason for hiding this comment

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

oh it's an IIFE

…bindings

Previously, the `previousValue` and `currentValue` arguments passed to the
`SimpleChange` constructor were swapped for interpolation bindings.

This commit also refactors the code, so that interpolation bindings and property
bindings share the same implementation, and fixes some broken tests (that hide
failures by allowing the `$exceptionHandler` to swallow thrown exceptions).
@gkalpak gkalpak force-pushed the fix-upgrade-correct-values-for-changes branch from ac17c78 to f5f3f8b Compare February 5, 2017 12:39
@gkalpak
Copy link
Member Author

gkalpak commented Feb 5, 2017

(Squashed the commits.)

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 5, 2017
mhevery pushed a commit to mhevery/angular that referenced this pull request Feb 7, 2017
…bindings (angular#14301)

Previously, the `previousValue` and `currentValue` arguments passed to the
`SimpleChange` constructor were swapped for interpolation bindings.

This commit also refactors the code, so that interpolation bindings and property
bindings share the same implementation, and fixes some broken tests (that hide
failures by allowing the `$exceptionHandler` to swallow thrown exceptions).

PR Close angular#14301
@mhevery mhevery closed this in 1e3dd3d Feb 7, 2017
@gkalpak gkalpak deleted the fix-upgrade-correct-values-for-changes branch February 7, 2017 19:40
@mhevery
Copy link
Contributor

mhevery commented Feb 9, 2017

@gkalpak this does not rebase on 2.4.x, if you would like me to merge it please create a new PR against that branch.

gkalpak added a commit to gkalpak/angular that referenced this pull request Feb 10, 2017
…bindings

Previously, the `previousValue` and `currentValue` arguments passed to the
`SimpleChange` constructor were swapped for interpolation bindings.

This commit also refactors the code, so that interpolation bindings and property
bindings share the same implementation, and fixes some broken tests (that hide
failures by allowing the `$exceptionHandler` to swallow thrown exceptions).

This is the same as angular#14301, but for the 2.4.x branch.
gkalpak added a commit to gkalpak/angular that referenced this pull request Feb 10, 2017
…bindings

Previously, the `previousValue` and `currentValue` arguments passed to the
`SimpleChange` constructor were swapped for interpolation bindings.

This commit also refactors the code, so that interpolation bindings and property
bindings share the same implementation, and fixes some broken tests (that hide
failures by allowing the `$exceptionHandler` to swallow thrown exceptions).

This is the same as angular#14301, but for the 2.4.x branch.
@gkalpak
Copy link
Member Author

gkalpak commented Feb 10, 2017

@mhevery, here is the PR: #14400

mhevery pushed a commit that referenced this pull request Feb 10, 2017
…bindings (#14400)

Previously, the `previousValue` and `currentValue` arguments passed to the
`SimpleChange` constructor were swapped for interpolation bindings.

This commit also refactors the code, so that interpolation bindings and property
bindings share the same implementation, and fixes some broken tests (that hide
failures by allowing the `$exceptionHandler` to swallow thrown exceptions).

This is the same as #14301, but for the 2.4.x branch.
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
…bindings (angular#14301)

Previously, the `previousValue` and `currentValue` arguments passed to the
`SimpleChange` constructor were swapped for interpolation bindings.

This commit also refactors the code, so that interpolation bindings and property
bindings share the same implementation, and fixes some broken tests (that hide
failures by allowing the `$exceptionHandler` to swallow thrown exceptions).

PR Close angular#14301
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
…bindings (angular#14301)

Previously, the `previousValue` and `currentValue` arguments passed to the
`SimpleChange` constructor were swapped for interpolation bindings.

This commit also refactors the code, so that interpolation bindings and property
bindings share the same implementation, and fixes some broken tests (that hide
failures by allowing the `$exceptionHandler` to swallow thrown exceptions).

PR Close angular#14301
@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
action: merge The PR is ready for merge by the caretaker cla: yes type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants