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

Adding SSR test for form fields. #9264

Merged
merged 2 commits into from Mar 31, 2017

Conversation

aickin
Copy link
Contributor

@aickin aickin commented Mar 27, 2017

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!

Copy link
Collaborator

@sophiebits sophiebits left a 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');
Copy link
Collaborator

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?

Copy link
Contributor Author

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

Up to you.

@aickin aickin merged commit 04713fc into facebook:master Mar 31, 2017
@aickin
Copy link
Contributor Author

aickin commented Mar 31, 2017

Thanks, merged! 🎉🎉🎉

@sophiebits
Copy link
Collaborator

🎉 :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants