Fixes initial option select

[yiminghe]

make sure initial select

<select value="2">
<option>1</option>
<option>2</option>
</select>

renders correctly.

Update is fine, because native option.value will get its content if value is absent

[facebook-github-bot]

@yiminghe updated the pull request.

[jimfb]

Looks good to me. Will leave it open for a day or two so everyone can take a look before we merge.

[sophiebits]

props.value != null please – we treat null and undefined the same as omitted props for all DOM components.

[sophiebits]

We should call getNativeProps and mountWrapper in the same order for all components – it is too confusing to call getNativeProps first in some cases and mountWrapper first in other cases.

[yiminghe]

Yes, I also feel weird for option to have a different order from others, then we have to call flattenChildren twice if value is omitted....

[facebook-github-bot]

@yiminghe updated the pull request.

[facebook-github-bot]

@yiminghe updated the pull request.

[jimfb]

If we warn inside flattenChildren, and flattenChildren gets called twice, it seems like we would warn twice, right? We might want to move the warning logic out of flattenChildren.

Also, let's squash into the two commits into a single commit before merging.

Otherwise, looks good to me. @spicyj?

[sophiebits]

Yeah. Can we just set a global flag so that we only warn once?

[yiminghe]

Just as before, only warn in getNativeProps

[jimfb]

@yiminghe Maybe I'm missing something, but isn't flattenChildren also now being called in mountWrapper?

[facebook-github-bot]

@yiminghe updated the pull request.

[yiminghe]

@jimfb add a flag to only warn in getNativeProps

https://github.com/yiminghe/react/blob/initial-option-select/src/renderers/dom/client/wrappers/ReactDOMOption.js#L20

[jimfb]

@yiminghe Sorry we dropped the ball on this one, but I'd like to try to get this merged. Can you rebase and add a unit test to demonstrate that it only warns once, and then I think we can merge.

[sophiebits]

Can you set a global (file-level) flag so that we only warn once, even if many children are the wrong type? Because we don't give information about the callsite it's not currently useful to warn many times.

A similar example is here:

react/src/renderers/dom/client/wrappers/ReactDOMTextarea.js

Line 43 in 1d49baf

didWarnValueNull = true;

Then you don't even need the needWarning parameter.

[yiminghe]

updated as requested

[gaearon]

Minor nit: missing space before (

[facebook-github-bot]

@yiminghe updated the pull request.

[sophiebits]

Thank you!