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

Ignore symbols and functions in select tag #13389

Merged
merged 4 commits into from Aug 14, 2018
Merged

Ignore symbols and functions in select tag #13389

merged 4 commits into from Aug 14, 2018

Conversation

@raunofreiberg
Copy link
Contributor

@raunofreiberg raunofreiberg commented Aug 14, 2018

Related to #11734 and #13362.

As expected, the same cases that fail for textarea seem to fail for select as well.

@raunofreiberg raunofreiberg changed the title wip: ignore symbols and functions in select tag Ignore symbols and functions in select tag Aug 14, 2018
@raunofreiberg
Copy link
Contributor Author

@raunofreiberg raunofreiberg commented Aug 14, 2018

I'm not sure whether we need to sanitize the value here since the initialValue seems to be set to undefined later on.

Loading

@@ -21,7 +23,7 @@ if (__DEV__) {

type SelectWithWrapperState = HTMLSelectElement & {
_wrapperState: {
initialValue: ?string,
initialValue: ToStringValue | void,
Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 14, 2018

Choose a reason for hiding this comment

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

Since ToStringValue is an opaque type I wasn't sure what to do here.

Loading

expect(selectNode.value).toBe('');
});

it('treats controlled function value as an empty string', () => {
Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 14, 2018

Choose a reason for hiding this comment

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

This seemed to be a additional case that I found failing before this PR. I don't know if this is the best way to test it, though.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 14, 2018

Choose a reason for hiding this comment

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

Actually, this case is probably fine as is, since the browser defaults to using the element's children as a value if the attribute value is not provided.

<select id="test">
  <option>hi</option>
</select>

console.log(document.getElementById('test').value) // 'hi'

Loading

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Just some style stuff. This looks great though.

I've left a note to myself to file an issue for my initialValue comment, but if you wanted to go ahead and investigate, it's all yours :)

Loading

<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
</select>,
)),
Copy link
Contributor

@nhunzaker nhunzaker Aug 14, 2018

Choose a reason for hiding this comment

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

Style nit: but I think the implicit return with an assignment distracts from the test. Could you remove them, like:

expect(() => {
  node = ReactTestUtils.renderIntoDocument(
    <select onChange={noop} value={Symbol('foobar')}>
      <option value={Symbol('foobar')}>A Symbol!</option>
      <option value="monkey">A monkey!</option>
      <option value="giraffe">A giraffe!</option>
    </select>
  )
}).toWarnDev('Invalid value for prop `value`');

Loading

@@ -21,7 +23,7 @@ if (__DEV__) {

type SelectWithWrapperState = HTMLSelectElement & {
_wrapperState: {
initialValue: ?string,
initialValue: ?ToStringValue,
Copy link
Contributor

@nhunzaker nhunzaker Aug 14, 2018

Choose a reason for hiding this comment

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

It's curious that this is nullable and it's not clear to me where initialValue is even used.

Not important for this PR, but I can't help but wonder if initialValue can go away.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 14, 2018

Choose a reason for hiding this comment

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

That's a good point. I'll try and play around with it and see what I find out 🙂Feel free to report an issue, regardless.

Loading

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Aug 14, 2018

Later today I'll do some manual testing just for good measure. Looks great though, 👍

Loading

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Aug 14, 2018

Checks out. Awesome work!

Loading

@nhunzaker nhunzaker merged commit de5102c into facebook:master Aug 14, 2018
1 check passed
Loading
@raunofreiberg raunofreiberg deleted the select-symbols-functions branch Aug 14, 2018
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* wip: ignore symbols and functions in select tag

* fix: Use ToStringValue as a maybe type

* refactor: remove unnecessary test

* refactor: remove implicit return from tests
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants