Select element update #7054

git-richard · 2016-06-16T17:30:41Z

I saw some others might be working on this issue but I'm submitting a PR to get some practice. Don't mean to step on any toes.

The code loops through the options in a select element and displays a warning if there are duplicate option values. There are more efficient array comparison algorithms but this works well. It shows all duplicate values rather than exiting the evaluation loops on the first dupe it finds.

Appreciate any feedback.

git-richard · 2016-06-16T17:45:01Z

Leave a comment

aweary · 2016-06-16T17:45:29Z

src/renderers/dom/client/wrappers/ReactDOMSelect.js

@@ -195,6 +217,9 @@ var ReactDOMSelect = {
      );
      didWarnValueDefaultValue = true;
    }
+
+    warnIfDuplicateValues(inst);


This should be placed within the _DEV_ block at the top of the function so it only runs in development.

jimfb · 2016-06-16T17:48:09Z

src/renderers/dom/client/wrappers/ReactDOMSelect.js

@@ -58,6 +58,28 @@ function warnIfValueIsNull(props) {
  }
 }

+function warnIfDuplicateValues(inst) {
+  const options = inst._currentElement.props.children;


I think it is legal for this to be a composite component, so the children might not be simple option tags.

They can also be nested inside arrays/maps/sets etc, so this does not work reliably.

Also not to forget that <optgroup> is a thing.

Also, children can be null or an element (not wrapped in an array). I'm not sure whether it is supported or not yet, but IIRC the idea is also to support anything that is iterable as valid value for children, so we can't assume this to be an array in that case?

ghost · 2016-06-16T17:50:13Z

@git-richard updated the pull request.

jimfb · 2016-06-16T17:51:12Z

src/renderers/dom/client/wrappers/ReactDOMSelect.js

+        if (options[i].props.value === options[j].props.value) {
+          warning(
+            false,
+            `Select element contains duplicate option value ${options[i].props.value} in options #${i} & #${j}`


We try to include a stack trace for new warnings. Again, #7040 is a good example.

…Update

git-richard · 2016-06-27T19:39:20Z

I updated the code based on previous feedback. Moved to the dev tools. It only processes controlled select elements. Processes option and optGroup children. Iterates the children instead of processing as an array. Shows the stack trace in the warning. A few other changes. Any feedback is appreciated.

hkal · 2016-06-30T22:35:41Z

src/renderers/dom/shared/devtools/ReactDOMDuplicateSelectValuesDevtool.js

+  }
+
+  // Check the array for duplicate values.
+  for (var i = 0; i < values.length-1; i++) {


This strategy for duplicate checking is a bit slow: O(n²)

We can check for duplicates in the first loop (line 52) by making values a dictionary and checking if the current value has already been seen:

const values = {}; for (const option of options) { ... // code for handling optgroups if (option.type === 'option' && option.props != null && option.props.value) { const value = option.props.value; if (!values[value]) { values[value] = value; } else { // warn about duplicate return; } } }

However, in reality, the chances of a <select> having enough <options>s to make this matter is small, but doing it this way does save us some lines of code.

Thanks for the feedback. I updated it to use the dictionary but found an issue so I reverted back to the arrays. The dictionary approach shows a warning for a select like this:

<select> <option value={{val: 'a'}}>a</option> <option value={{val: 'b'}}>b</option> </select>

Those are different values and shouldn't show a warning. I agree that a select shouldn't normally have enough options for the array comparisons to be a performance issue.

@git-richard The value is stringified (inspect the DOM) so in your case they are actually the same.

You're right. I didn't notice that. Do you think using a dictionary of the options, as mentioned above, is a correct approach? Thanks,

If we can avoid O(n^2) definitely seems favorable I'd say. EDIT: For most it probably doesn't matter, but consider it's just a warning, ensuring it never balloons seems like a good idea.

I changed it back to a dictionary. Do you see any other issues with it?

syranide · 2016-07-01T19:16:58Z

src/renderers/dom/shared/devtools/ReactDOMDuplicateSelectValuesDevtool.js

+    if (option.type === 'option') {
+      if ((option.props != null) && (option.props.value)) {
+        const value = option.props.value;
+        if (!values[value]) {


Needs to use hasOwnProperty. I'm not sure if it really matters, but it may make sense to explicitly stringify value as well?

chicoxyzzy · 2016-07-05T18:01:10Z

~~Still don't sure this will solve the problem as mentioned in #6959 (comment)~~

LGTM

zeel · 2016-07-06T08:54:15Z

src/renderers/dom/shared/devtools/ReactDOMDuplicateSelectValuesDevtool.js

+    }
+
+    if (option.type === 'optGroup') {
+      if ((option.props != null) && (option.props.children != null)) {


it would be great if we can merge this two ifs.

montogeek · 2017-09-25T17:11:46Z

It is this still relevant?

aweary · 2017-09-25T18:25:45Z

@montogeek nope, these files don't exist anymore. Thanks for the heads up!

gaearon · 2017-09-25T19:05:24Z

What was the relevant issue?

montogeek · 2017-09-25T19:15:11Z

From the original message: "...displays a warning if there are duplicate option values." Then he introduced an array comparison algorithm

aweary · 2017-09-25T19:42:02Z

The issue this was meant to address is #6959.

@git-richard I'm sorry we didn't provide any feedback on this. Sometimes low priority issues slip through the cracks. It's been over a year since this PR was opened, and the codebase has seen substantial changes. If you want to keep working on this, I would suggest looking at ReactDOMFiberComponent and how it uses modules like ReactDOMUnkownPropertyHook for DEV-only warnings.

Feel free to open a new PR for this if you do choose to pursue it, and also feel free to ping us with questions. Thanks!

git-richard added 2 commits June 16, 2016 02:55

Check for duplicate option values in select element.

021a6ca

Check for duplicate option values in select element.

cc7b610

ghost added GH Review: review-needed CLA Signed labels Jun 16, 2016

git-richard closed this Jun 16, 2016

aweary reviewed Jun 16, 2016
View reviewed changes

git-richard reopened this Jun 16, 2016

Moved to the __DEV__ block.

87587e2

jimfb reviewed Jun 16, 2016
View reviewed changes

git-richard added 4 commits June 21, 2016 02:03

Update

dc09140

Update

0a6cf21

Updates

d092c52

Merge branch 'master' of github.com:facebook/react into SelectElement…

dbd4851

…Update

gaearon added GH Review: needs-revision and removed GH Review: review-needed labels Jun 26, 2016

Check for duplicate values in select element

bf9fe29

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 27, 2016

git-richard added 2 commits June 27, 2016 11:38

Check for duplicate values in select element.

01c63d5

Fixed warning message

4c932a0

Early return if warning has already been displayed.

06aafde

hkal reviewed Jun 30, 2016
View reviewed changes

git-richard added 3 commits June 30, 2016 20:29

Put values in a dictionary instead of an array.

f129956

Changed option values dictionary back to an array.

1fd6935

Put option values in a dictionary.

c172e1c

syranide reviewed Jul 1, 2016
View reviewed changes

git-richard added 3 commits July 3, 2016 01:53

Updates & removed redundant code

03dc300

Changed a 'let' to 'const'

36524f3

Removed function parameters..

0f38d16

syranide mentioned this pull request Jul 5, 2016

Should React warn when controlled <select> components have duplicate values? #6959

Closed

zeel reviewed Jul 6, 2016
View reviewed changes

Consolidated 'if' statements.

1445899

chicoxyzzy mentioned this pull request Jul 6, 2016

<select> behaviour should be consistent with DOM when <option>s values are identical #7203

Closed

ghost added the CLA Signed label Jul 12, 2016

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

aweary closed this Sep 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select element update #7054

Select element update #7054

git-richard commented Jun 16, 2016

git-richard commented Jun 16, 2016

aweary Jun 16, 2016

jimfb Jun 16, 2016

syranide Jun 18, 2016

syranide Jun 18, 2016

syranide Jun 18, 2016 •

edited

Loading

ghost commented Jun 16, 2016

jimfb Jun 16, 2016

git-richard commented Jun 27, 2016

hkal Jun 30, 2016

git-richard Jul 1, 2016

syranide Jul 1, 2016

git-richard Jul 1, 2016

syranide Jul 1, 2016 •

edited

Loading

git-richard Jul 1, 2016

syranide Jul 1, 2016 •

edited

Loading

chicoxyzzy commented Jul 5, 2016 •

edited

Loading

zeel Jul 6, 2016

montogeek commented Sep 25, 2017

aweary commented Sep 25, 2017

gaearon commented Sep 25, 2017

montogeek commented Sep 25, 2017

aweary commented Sep 25, 2017

Select element update #7054

Select element update #7054

Conversation

git-richard commented Jun 16, 2016

git-richard commented Jun 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide Jun 18, 2016 • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 16, 2016

Choose a reason for hiding this comment

git-richard commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide Jul 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide Jul 1, 2016 • edited Loading

Choose a reason for hiding this comment

chicoxyzzy commented Jul 5, 2016 • edited Loading

Choose a reason for hiding this comment

montogeek commented Sep 25, 2017

aweary commented Sep 25, 2017

gaearon commented Sep 25, 2017

montogeek commented Sep 25, 2017

aweary commented Sep 25, 2017

syranide Jun 18, 2016 •

edited

Loading

syranide Jul 1, 2016 •

edited

Loading

syranide Jul 1, 2016 •

edited

Loading

chicoxyzzy commented Jul 5, 2016 •

edited

Loading