fix(upgrade): two-way binding and listening for event #22772
Conversation
a681727 to
97bedce
Compare
There was a problem hiding this comment.
The change LGTM 👍
The added test only tests the (now deprecated) dynamic version of ngUpgrade. We mainly need tests for the static version (with both UpgradeModule and downgradeComponent()); e.g. in downgrade_component_spec.ts and downgrade_module_spec.ts.
(Also, that particular test you modified is already very hard to follow. It would be nice if you could add new tests specifically focused on the fix 🙏)
| emitter.subscribe({ | ||
| next: assignExpr ? (v: any) => setter !(this.scope, v) : | ||
| private subscribeToOutput( | ||
| output: PropertyBinding, expr: string|null, isAssignment: boolean = false) { |
There was a problem hiding this comment.
expr shouldn't be null at this point. Can you change the type (and also remove the check below)?
There was a problem hiding this comment.
I'll check if that doesn't cause any issues as the if before calling this only tests if the property is there, not that it's not null
There was a problem hiding this comment.
It would be very weird for attrs.someProp to be set to null (as one would have a directive to specifically do that, which doesn't make much sense) and this would only matter is isAssignment is true. But you have a point; there is no good reason to unnecessarily break a weird usecase 😁
Let's leave it as is.
There was a problem hiding this comment.
That is true, and very unlikely - I hope you don't mind that I've removed the check.
|
Yeah, that test is quite something. Sad part is that it was the only one that failed if setupOutputs method did nothing, hence why I've changed it. I'll try to update the specs you mentioned and cover the code I've changed later today. |
97bedce to
1eab1f3
Compare
|
I've reverted the test in dynamic version, added a new spec in |
There was a problem hiding this comment.
LGTM, except it would be nice to have a similar test for the dynamic version as well.
|
Also, the commit message is a little confusing in that it makes the bug sound specific to |
Changes would not propagate to a value in downgraded component in case you had two-way binding and listening to a value-change, e.g. [(value)]="value" (value-change)="fetch()" Closes angular#22734
1eab1f3 to
5b41d62
Compare
|
I've updated the message a bit and also copied and adapted the test to ensure that dynamic version is also working |
|
LGTM, thx 👍 |
Changes would not propagate to a value in downgraded component in case you had two-way binding and listening to a value-change, e.g. [(value)]="value" (value-change)="fetch()" Closes angular#22734 PR Close angular#22772
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
It is not possible to have a two-way binding and listening to an event in downgraded component. Bound value would remain outdated and only event listener expression would be evaluated.
Issue Number: #22734
What is the new behavior?
Bound value will be updated first when new value is emitted and then expression (if any) will be evaluated.
Does this PR introduce a breaking change?
Other information