Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false positives for display-name #2399

Open
wants to merge 1 commit into
base: master
from

Conversation

@BPScott
Copy link

commented Sep 3, 2019

Wrapping a named function declaration with a React.memo or
React.forwardRef will no longer throw false positive errors.

Check how React devtools exposes the displayNames of components wrapped in memo and forwardRef in https://codesandbox.io/s/awesome-paper-hh0wh?module=App.js

The following no longer raise a linting warning (as react can determine displayNames for them):

import React from 'react'

export const ComponentWithMemo = React.memo(function Component({ world }) {
 return <div>Hello {world}</div>
})

export const ComponentWithForwardRef = React.forwardRef(function Component({ world }) {
  return <div>Hello {world}</div>
})

Using either an unnamed Function declaration (function() {/*...*/}) or arrow functions continue to raise a warning, as a displayName can not be inferred.

Nesting memo and forwardref (const Component = React.memo(React.forwardRef(function Component() { /*...*/})) is not currently handled by react so continue to raise an error in that case

Fixes #2324, #2269

@@ -90,13 +90,13 @@ module.exports = {
const namedClass = (
(node.type === 'ClassDeclaration' || node.type === 'ClassExpression') &&
node.id &&
node.id.name
!!node.id.name

This comment has been minimized.

Copy link
@BPScott

BPScott Sep 3, 2019

Author

This and the below addition of !! aren't strictly needed but I spotted every other variable in this block returns a boolean so I figured these should too

import React from 'react'
export const ComponentWithMemoAndForwardRef = React.memo(
React.forwardRef(({ world }) => {

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 3, 2019

Collaborator

the thing inside forwardRef is not a component, and should not be identified as such. forwardRef callbacks take props and ref, which isn't what components take.

This comment has been minimized.

Copy link
@BPScott

BPScott Sep 3, 2019

Author

Gotcha, thanks for the clarification :)

In that case I think there is something off with the Component detection, as this testcase identifies two components (as found by doing a console.log(components.list()) in the Program:exit function in the display-name rule). It sounds like it should only identify one - possibly the outer call to React.memo. This also occurs on master too.

At risk of creating one of those magic eye "how many triangles can you see puzzles", how many components should be identified in the following pieces of code?

// Currently reports 1 component - I *think* that is the result of calling React.memo, and the argument that is passed into React.memo is ignored, can you confirm?
const ComponentWithMemo = React.memo(function Component({ world }) {
  return <div>Hello {world}</div>
})
// currently reports 1 component - The result of calling React.forwardRef is a component, the argument that is passed into React.forwardRef isn't a component
const ComponentWithForwardRef = React.forwardRef(function NotAComponent({ world }, ref) {
  return <div>Hello {world}</div>
}, )
// Should this report one or two components found?
// Currently it seems to identify two, - The result of calling React.memo, and the result of React.forwardRef, but that seems at odds with how React.memo worked previously where it didn't count the component that was the argument passed into React.memo
const ComponentWithMemoAndForwardRef = React.memo(React.forwardRef(function Component({ world }, ref) {
  return <div>Hello {world}</div>
}))

I don't understand how the internal Component identification heuristics works and what knock-on effects changing them would have. It looks like 98d4bf3 did some work relating to this but I'm not sure how to fix my above confusion

}, {
// React does not currently set a displayName on the memo function when you
// pass it a value from forwardRef.
// Hopefully eventually that will be fixed, but until then flagging this

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 3, 2019

Collaborator

this shouldn't be fixed because it's not broken; forwardRef is a component but does not accept one; it accepts a callback that takes props and ref.

This comment has been minimized.

Copy link
@BPScott

BPScott Sep 3, 2019

Author

I think I'm referring to a slightly different issue. I'm referring how React.memo handles accepting a Component vs the result of calling forwardRef.

When inspecting the MemoFunction and MemoThenForwardRefFunction
items on https://codesandbox.io/s/awesome-paper-hh0wh?module=App.js I end up with the following in my react devtools:

Screen Shot 2019-09-02 at 11 28 43 PM

Note that calling React.memo with a function component defined inline

const MemoFunction = React.memo(function MemoFunction() {
  return <div>MemoFunction</div>;
});

Results in in the name MemoFunction [memo] in the component tree.

However calling React.memo with an invocation of React.forwardRef (which returns a Component)

const MemoThenForwardRefFunction = React.memo(
  React.forwardRef(function MemoThenForwardRefFunction() {
    return <div>MemoThenForwardRefFunction</div>;
  })
);

Results in the name Anonymous in the component tree - Given that the result of an unwrapped call to React.forwardRef has a meaningful name, I would have expected that to have been picked up by React.memo

This comment has been minimized.

Copy link
@BPScott

BPScott Sep 17, 2019

Author

I've raised this upstream as facebook/react#16722

@BPScott
Copy link
Author

left a comment

.

@BPScott BPScott force-pushed the BPScott:fix-display-name-false-positives branch 4 times, most recently from 0ea2ea4 to 9b7613f Sep 17, 2019

Fix false positives for display-name
Wrapping a named function declaration with a React.memo or
React.forwardRef will no longer throw an false positive error

@BPScott BPScott force-pushed the BPScott:fix-display-name-false-positives branch from 9b7613f to 9b5292e Sep 17, 2019

@BPScott

This comment has been minimized.

Copy link
Author

commented Sep 17, 2019

Heya @ljharb, I've gave this a kick again and I think it is ready for review. I've updated the naming in the tests.

The functions that you pass into forwardRef are trixy - they're not quite components as you say, but you can set a displayName on them and that gets honored like regular components. Because they act like components in that sense I think it makes sense to use the existing componentt detection logic for them.

I'm torn on what to do when we have a nested React.memo(React.forwardRef(/*...*/)). It's marked as an error for now, but we can go back and revisit later.

return <div ref={ref}>Hello {world}</div>
})
`,
parser: parsers.BABEL_ESLINT,

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 17, 2019

Collaborator

does this need babel-eslint?

return <div ref={ref}>Hello {world}</div>
})
`,
parser: parsers.BABEL_ESLINT,

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 17, 2019

Collaborator

does this need babel-eslint?

return <div>Hello {world}</div>
})
`,
parser: parsers.BABEL_ESLINT,

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 17, 2019

Collaborator

does this need babel-eslint?

return <div>Hello {world}</div>
})
`,
parser: parsers.BABEL_ESLINT,

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 17, 2019

Collaborator

does this need babel-eslint?

})
)
`,
parser: parsers.BABEL_ESLINT,

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 17, 2019

Collaborator

does this need babel-eslint?

})
)
`,
parser: parsers.BABEL_ESLINT,

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 17, 2019

Collaborator

does this need babel-eslint?

})
)
`,
parser: parsers.BABEL_ESLINT,

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 17, 2019

Collaborator

does this need babel-eslint?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.