fix: React DOM crashes when <option> contains three interpolated valu…

[stephenkingsley]

fix #11911
…e if one is a conditional. #11911

Before submitting a pull request, please make sure the following is done:

1. Fork the repository and create your branch from master.
2. Run yarn in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (yarn test). Tip: yarn test --watch TestName is helpful in development.
5. Run yarn test-prod to test in the production environment. It supports the same options as yarn test.
6. If you need a debugger, run yarn debug-test --watch TestName, open chrome://inspect, and press "Inspect".
7. Format your code with prettier (yarn prettier).
8. Make sure your code lints (yarn lint). Tip: yarn linc to only check changed files.
9. Run the Flow typechecks (yarn flow).
10. If you haven't already, complete the CLA.

Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[gaearon]

This is a behavior change. I don't think we want this.

[gaearon]

Please:

1. Don't change the existing behavior (apart from from fixing a bug). It should match how this worked in React 15.

2. Add a test that would have failed without your fix.

[stephenkingsley]

Sorry that is my fault! I will fix it in today.

[stephenkingsley]

@gaearon please review again

[stephenkingsley]

@gaearon Would you have anytime to review, please?

[gaearon]

Can you explain what this is doing? It’s not obvious from the code that it is correct. Generally when submitting a PR it is a good idea to explain how you arrived at this conclusion, when it got broken, whether you can point to React 15 code doing a similar thing, etc.

[stephenkingsley]

My english is not good enough to explain how I do this.

When render <option>{1}<div />{2}</option>, Option has three children like [textNode, Div, textNode], so I use reduceOptionChild function to remove Div. Because the Div is illegal in option.

If render like this, when change the value, option has one child that is textNode. So react was crash and warning Node was not found. I find out function assertValidProps change this option's child. Before change, this option has three childNodes. When run assertValidProps, the option has one childNode: textNode. So I use span contain textNodes.

<option key={value} value={value}>
  {isSelected && '(Y) '}
  {value} 
  {'Some string'}
</option>

I think you're right, I should do something like React 15 code.

[gaearon]

My point is:

• If this is a regression from React 15 behavior, please implement whatever React 15 did.
• If this has always been present then let's discuss possible solutions separately.

[aweary]

I only looked at this briefly, but I think there may be a deeper issue with child reconciliation. Specifically this logic (or maybe somewhere else upstream) in commitPlacement

react/packages/react-reconciler/src/ReactFiberCommitWork.js

Lines 630 to 648 in 3596e40

	const before = getHostSibling(finishedWork);
	// We only have the top Fiber that was inserted but we need recurse down its
	// children to find all the terminal nodes.
	let node: Fiber = finishedWork;
	while (true) {
	if (node.tag === HostComponent || node.tag === HostText) {
	if (before) {
	if (isContainer) {
	insertInContainerBefore(parent, node.stateNode, before);
	} else {
	insertBefore(parent, node.stateNode, before);
	}
	} else {
	if (isContainer) {
	appendChildToContainer(parent, node.stateNode);
	} else {
	appendChild(parent, node.stateNode);
	}
	}

before should always be a child of parent, and in this case it isn't. I'm not exactly sure why that's the case yet, but my gut feeling is that this solution is fixing a symptom not the problem.

[gaearon]

This was fixed in #13261. Thanks!