Skip to content

feat(core): add migration for invalid two-way bindings#54630

Closed
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:invalid-two-way-migration
Closed

feat(core): add migration for invalid two-way bindings#54630
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:invalid-two-way-migration

Conversation

@crisbeto
Copy link
Member

As a part of #54154, an old parser behavior came up where two-way bindings were parsed by appending = $event to the event side. This was problematic, because it allowed some non-writable expressions to be passed into two-way bindings.

These changes introduce a migration that will change the two-way bindings into two separate input/output bindings that represent the old behavior so that in a future version we can throw a parser error for the invalid expressions.

// Before
@Component({
  template: `<input [(ngModel)]="a && b"/>`
})
export class MyComp {}

// After
@Component({
  template: `<input [ngModel]="a && b" (ngModelChange)="a && (b = $event)"/>`
})
export class MyComp {}

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: migrations Issues related to `ng update`/`ng generate` migrations target: minor This PR is targeted for the next minor release labels Feb 27, 2024
@ngbot ngbot bot added this to the Backlog milestone Feb 27, 2024
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Feb 27, 2024
@crisbeto crisbeto requested a review from devversion February 27, 2024 17:53
@crisbeto crisbeto marked this pull request as ready for review February 27, 2024 17:53
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Nice— and wow. The behavior is pretty magical

@crisbeto crisbeto force-pushed the invalid-two-way-migration branch 2 times, most recently from 4f26c6c to ba4ded1 Compare February 28, 2024 14:21
@crisbeto crisbeto added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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 28, 2024
As a part of angular#54154, an old parser behavior came up where two-way bindings were parsed by appending `= $event` to the event side. This was problematic, because it allowed some non-writable expressions to be passed into two-way bindings.

These changes introduce a migration that will change the two-way bindings into two separate input/output bindings that represent the old behavior so that in a future version we can throw a parser error for the invalid expressions.

```ts
// Before
@component({
  template: `<input [(ngModel)]="a && b"/>`
})
export class MyComp {}

// After
@component({
  template: `<input [ngModel]="a && b" (ngModelChange)="a && (b = $event)"/>`
})
export class MyComp {}
```
@crisbeto crisbeto removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Feb 28, 2024
@crisbeto crisbeto force-pushed the invalid-two-way-migration branch from ba4ded1 to f749358 Compare February 28, 2024 16:17
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit fb540e1.

@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 Mar 30, 2024
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 area: migrations Issues related to `ng update`/`ng generate` migrations detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants