ReactDOMSelect makeover, fix edge-case inconsistencies and remove state

[syranide]

Broken behavior discovered/encountered by #1510

EDIT: I found a handful more broken edge-cases so I decided to do a large makeover instead, simplifying the logic and removing state entirely.

I've intentionally made it "reset to default" if you toggle multiple because it simplified the code and I don't think it's a sane transition (browsers don't agree on what should happen either). But I could easily add it back if you disagree, but I figured it wasn't worth the size and complexity cost.

As far as I'm aware, all edge-cases should be fixed now.

Previously, state would only be updated on user interaction or when toggling multiple. This meant that any forced value update was not remembered when toggling multiple or becoming uncontrolled.

To solve this it has to read value during componentWillReceiveProps, which LinkedValueUtils couldn't do. So I updated LinkedValueUtils to take a props-object instead, which seems consistent with the direction of a reactjs-future proposal as well.

• Test plan: grunt test and cursory manual testing
• Poach test-case from Fix issue where updating <option> children of <select> element doesn't trigger getting selected attribute properly set. #2289

   raw     gz Compared to master @ 4f27dc30ad065856c9ed7de9c4dd8366801b9486
  -215    -85 build/react-with-addons.js
  -166    -92 build/react-with-addons.min.js
  -215    -83 build/react.js
  -166    -97 build/react.min.js

[syranide]

Hmm, there are even more broken edge-cases relating to toggling multiple. One idea is to consider another case of #2242 (if you agree with it), i.e. treat <select> and <select multiple> as different base components, toggling multiple would then remount instead.

But perhaps that's a bad idea and simply fixing ReactDOMSelect is preferable.

[syranide]

cc @zpao @spicyj

I rewrote most of ReactDOMSelect, updated OP with info too.

[sebmarkbage]

Also, cc @yungsters

[zpao]

This needs a rebase but I don't think that'll change much of the code... @yungsters!

[syranide]

@zpao Rebased, it was just setImmediate => asap.

[yungsters]

Looks like the type of propValue should be {*} because we should never get null or undefined with the new code, right?

Also, maybe s/string/stringable/g.

[syranide]

Correct and sounds good. Fixed.

[yungsters]

Looks pretty straightforward. Couple questions inline, and I also want to clarify... besides changing the behavior of switching up multiple (which I think is fine), what else does this do? In particular, can you describe the edge cases the title is referring to?

[syranide]

@yungsters As for the edge-cases, the biggest ones I found where relating to the state being kept. If you told a controlled select to select 2 options and one didn't exist, if you then added that option it would not be selected. There were weird issues when toggling multiple where it would incorrectly apply old selections instead and other edge-cases of similar nature.

Don't remember all of them or the specifics, I just dug deep enough to see that the state being kept was flawed and made it really hard to understand what was actually going on.

[yungsters]

Changes look good to me.

[sebmarkbage]

Thanks for the review @yungsters! Appreciate it.

[syranide]

I poached the test-case from #2289 as well then, but changed it to use value instead because it didn't make sense for defaultValue.

[zpao]

🙏