-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Adding SSR test for form fields. #9264
Adding SSR test for form fields. #9264
Conversation
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.
Nice! These didn't feel too clumsy to me.
<input value="foo" defaultValue="bar" readOnly={true} />, | ||
1, | ||
); | ||
expect(e.value).toBe('foo'); |
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.
Do you want to check e.getAttribute('value') too?
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.
Good call. I added those checks in a734aec. Thanks!
itRenders( | ||
'an input with a value and no onChange/readOnly', | ||
async render => { | ||
// this configuration should raise a dev warning that value without |
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 suppose it's not clear to me whether we should try to keep all the warnings in sync between the renderers. It seems not-super-valuable if you always revive on the client side, though I suppose if we want to support server-only rendering then we should have all the warnings there too. In any event, this seems good for now.
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 agree; I'm genuinely not certain if it's a good idea to have in SSR or not. If not, we can just change these tests pretty easily, though.
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.
Up to you.
Thanks, merged! 🎉🎉🎉 |
🎉 :D |
This is the last (🎉😢) of my planned SSR unit test PRs (following on #9055, #9089, #9106, #9221, and #9257). It tests form fields:
<input type=text>
,<input type=checkbox>
,<textarea>
, and<select>
.The first set of tests makes sure that the server renderer correctly maps React properties for each of those fields to correct DOM properties (or for
textarea
, to inner text). It also makes sure that the renderer warns when a value is set without onChange or readOnly.The latter tests attempt to make sure that form fields that have been server rendered work correctly with user interaction, whether they are controlled or uncontrolled and whether the user interaction happens before client render or after.
I also deleted a duplicative unrelated test that @spicyj mentioned in a review comment on #9257.
This set of tests are more verbose and a little more complex than some of the others; I welcome any feedback to make them more readable. Thanks!