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

create-react-app React performance with SVG as ReactComponent and eslint with React.memo missing display name #26

Closed
frederikhors opened this issue May 1, 2019 · 7 comments
Labels
good first issue Good for newcomers question Further information is requested

Comments

@frederikhors
Copy link

I'm using this component in a create-react-app app:

import { ReactComponent as ProfileIcon } from "./icons/profile.svg";

...
render(){
  <ProfileIcon {...props}/>
}

Using also why-did-you-render (https://github.com/welldone-software/why-did-you-render) I got this warning:

SvgProfileIcon
whyDidYouRender.min.js:1191 {SvgProfileIcon: ƒ} "Re-rendered because the props object itself changed but it's values are all equal." "This could of been avoided by making the component pure, or by preventing it's father from re-rendering." "more info at http://bit.ly/wdyr02"
whyDidYouRender.min.js:1191 prev props: {svgRef: null, className: "icon", height: "24", width: "24"} !== {svgRef: null, className: "icon", height: "24", width: "24"} :next props

So I made a custom PureComponent like this:

import React from "react";

export default WrappedComponent =>
  React.memo(props => <WrappedComponent {...props} />, () => true);

FIRST QUESTION: Is this performances-correct?

I'm using it like this:

import { ReactComponent as ProfileIcon } from "./icons/profile.svg";

import PureComponent from "./PureComponent";

const PureProfileIcon = PureComponent(ProfileIcon);

...
render(){
  <PureProfileIcon {...props}/>
}

SECOND QUESTION: Can I avoid this component at all using React.memo (or something else) differently?

Now eslint is complaining about:

Component definition is missing display name eslint(react/display-name)

THIRD QUESTION: How can I fix this?

@vzaidman
Copy link
Collaborator

vzaidman commented May 1, 2019

whats the point of

React.memo(props => <WrappedComponent {...props} />, () => true);

?
it makes it so every props, no metter what, would cause WrappedComponent to be recreated.

EDIT: i got it, it means it should never re-render because "areEqual" returns "true"

@vzaidman vzaidman added good first issue Good for newcomers question Further information is requested labels May 1, 2019
@frederikhors
Copy link
Author

Sorry, I don't understand. I'm using React.memo to avoid re-render and the second argument is a callback which if true skip the props check.

Am I right?

@frederikhors
Copy link
Author

I think something like shouldComponentUpdate() { return false }.

Am I right?

@ljosberinn
Copy link

Yes, that's correct, the function passed as 2nd argument to useMemo() is usually called areEqual, thus it semantically makes sense to not re-render if areEqual returns true.
See https://reactjs.org/docs/react-api.html#reactmemo for reference.

You can fix the eslint/display-name with

const PureProfileIcon = PureComponent(ProfileIcon);
PureProfileIcon.displayName = 'PureProfileIcon';

but that rule is purely useful during development as production will have names minified anyways so.

@vzaidman
Copy link
Collaborator

vzaidman commented May 2, 2019

i got it, it means it should never re-render because "areEqual" returns "true"

@vzaidman
Copy link
Collaborator

vzaidman commented May 2, 2019

for the warning:
"Re-rendered because the props object itself changed but it's values are all equal"
using React.memo should be enough:

export default WrappedComponent =>
  React.memo(props => <WrappedComponent {...props} />); // no () => true

unless you dont want the component to ever re-render. then you indeed can add () => true like you did:

export default WrappedComponent =>
  React.memo(props => <WrappedComponent {...props} />, () => true);

I wouldn't call it "PureComponent" though.

If you want a pure component, use React.memo in the first way.

If a component never re-renders, you can call it:
StaticComponent or even NeverReRender or something.

@frederikhors
Copy link
Author

@vzaidman, @ljosberinn thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants