Skip to content

Commit

Permalink
jsx-eslint#969 fix: exception for jsx-no-duplicate-props
Browse files Browse the repository at this point in the history 
jsx-no-duplicate-props is causing error. 
TypeError: name.toLowerCase is not a function. 
When <Element {...props}> is used.
  • Loading branch information
@marcelmokos
marcelmokos authored Jun 4, 2017
1 parent 11117a4 commit 6918bba
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/rules/jsx-no-duplicate-props.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ module.exports = {

var name = decl.name.name;

if (ignoreCase) {
if (ignoreCase && typeof name === 'string') {
name = name.toLowerCase();
}

Expand Down

7 comments on commit 6918bba

@sompylasar
Copy link

Choose a reason for hiding this comment

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

@marcelmokos @DianaSuvorova

This change is a workaround, not a fix. This line below props[name] = 1; will evaluate to props["[object Object]"] = 1; that is not intended.

The right change would be to handle the string and the object cases for all the code below the var name = decl.name.name;.

What AST node stands for the object in the decl.name.name?

@marcelmokos
Copy link
Owner Author

Choose a reason for hiding this comment

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

The problem is that when the code is invalid it creates an error that makes no sense.

Currently, the fix is sufficient. You are proposing to implement a recursive function to find a name that is actually a string in the nested object.

@sompylasar
Copy link

Choose a reason for hiding this comment

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

I'm proposing to at least wrap every line of code that is data-dependent on the name variable into the type check you introduced.

@sompylasar
Copy link

Choose a reason for hiding this comment

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

@marcelmokos This will prevent further lines that depend on name being a string from running.

         node.attributes.forEach(function(decl) {
           if (decl.type === 'JSXSpreadAttribute') {
             return;
           }
 
           var name = decl.name.name;
+
+          if (typeof name !== 'string') {
+            return;
+          }
 
-          if (ignoreCase && typeof name === 'string') {
+          if (ignoreCase) {
             name = name.toLowerCase();
           }
 
           if (has(props, name)) {
             context.report({
               node: decl,
               message: 'No duplicate props allowed'
             });
           } else {
             props[name] = 1;
           }
         });

@DianaSuvorova
Copy link

Choose a reason for hiding this comment

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

@marcelmokos Oh I see what you mean.

So proposed test case might be something like:

const invalidJSX = () => {
  return (
    <div>
      <svg >
        <use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#myIcon"></use>
      </svg>
      <svg >
        <use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#myAnotherIcon"></use>
      </svg>
    </div>
  );
};

With your change linter will skip both invalid JSX instances. With the original change it may report
'No duplicate props allowed' because it will consider "[object Object]" props duplicated.

@marcelmokos do you want to take care of updating your code and creating a PR?

@sompylasar
Copy link

@sompylasar sompylasar commented on 6918bba Jul 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@DianaSuvorova Exactly. Sorry it was hard for me to type such test cases from a phone.

I also think not seeing data flows and strict type errors in JavaScript is the cause for such patchy fixes. If this were written in TypeScript or with other typechecking, be it Flow or Elm, the compiler/linter would immediately say that the type of name can be object or string but the variable is always used as a string.

This can be inferred from the ESTree JSXOpeningElement specification:

  • attributes: [ESTree.JSXAttribute.t | ESTree.JSXSpreadAttribute.t], so that an element of the node.attributes array, which is decl, is of type ESTree.JSXAttribute.t | ESTree.JSXSpreadAttribute.t,
  • we skipped JSXSpreadAttribute type with if (decl.type === 'JSXSpreadAttribute') { return; },
  • so we're left with decl being of type JSXAttribute, for which name: ESTree.JSXIdentifier.t | ESTree.JSXNamespacedName.t (that is our decl.name),
  • so decl.name is of type ESTree.JSXIdentifier.t | ESTree.JSXNamespacedName.t which has two cases:
    1. decl.name is JSXIdentifier, then it has name: binary – this is our decl.name.name being a string case)
    1. decl.name is JSXNamespacedName then it has name: ESTree.JSXIdentifier.t – this is our decl.name.name being an object (the JSXIdentifier AST node).

@sompylasar
Copy link

Choose a reason for hiding this comment

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

Edited 6918bba#commitcomment-23209210 – fixed a typo in the type inference explanation.

Please sign in to comment.