-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
feat(core): Support two-way binding for value
on Input
elements
#55873
base: main
Are you sure you want to change the base?
Conversation
ddca31c
to
f83703b
Compare
f83703b
to
67d5739
Compare
*/ | ||
function mapInput2WayBinding(nativeElement: RElement, eventName: string) { | ||
if (nativeElement.tagName === 'INPUT' && eventName === 'valueChange') { | ||
return 'input'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the semantics may be confusing. e.g. Why input and not the change event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input
event fires on every keystroke, change
only fires on blur/enter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but why do we choose input and not change? In addition, how can a user tell what event they will get.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would probably mention that in the docs.
About input
vs change
, we're binding the value
property on an HTMLInputElement
and the value of this property changes on every input
event. I would expect the signal to reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, people might have different expectations- so we should make sure we considering the alternatives as well.
I definitely also do see input valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Input events also feel like the right choice to me, since this is how ngModel behaves, so it's "consistent."
@@ -248,6 +250,29 @@ describe('event listeners', () => { | |||
button.click(); | |||
expect(fixture.componentInstance.comp).toBeInstanceOf(MyComp); | |||
}); | |||
|
|||
it('should support 2-way binding on input value', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have some tests that cover the more complex input types, like date or number? Also we'd probably need some tests for the template type checking side as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value
property will always return a string, we could supporting binding on valueAsNumber
& valueAsDate
to support those types.
d4e2c65
to
1761ea5
Compare
The commit adds runtime support for two-way bindings on input. No Forms imports required this is supported at runtime level. The compiler didn't throw any warnings on value 2-way bindings before, so no changes on this side.
1761ea5
to
2f2e354
Compare
function createTwoWayBindingEventMapping( | ||
nativeElement: RElement, | ||
eventName: string, | ||
): {mapperFn: (e: any) => any; eventName: string} | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we optimize this structure by making it a tuple ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO not necessary.
The commit adds runtime support for two-way bindings on input. No Forms imports required this is supported at runtime level. This will only handle strings as
value
returns astring
.The compiler didn't throw any warnings on value 2-way bindings before, so no changes on this side.