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

React version "detect" is slow, particularly for no-unknown-property #2671

Closed
lencioni opened this issue Jun 15, 2020 · 2 comments · Fixed by #2673
Closed

React version "detect" is slow, particularly for no-unknown-property #2671

lencioni opened this issue Jun 15, 2020 · 2 comments · Fixed by #2673

Comments

@lencioni
Copy link
Collaborator

I've been timing and profiling ESLint runs at Airbnb and noticed that react/no-unknown-property is particularly slow for us when using the "detect" setting for React version.

When running ESLint using TIMING=1 on a directory that contains about 500 files to be linted in it, react/no-unknown-property shows up as taking about 1700ms.

Looking at the callstacks in the profiler, it seems that when this rule calls getStandardName for every JSXAttribute here, it will actually do all of the work to detect the React version every time to determine the correct set of DOM property names.

https://github.com/yannickcr/eslint-plugin-react/blob/ef9a51225e3d5f6cb7cbdc3ec0382ad49246034b/lib/rules/no-unknown-property.js#L275

image

By specifying the react version in our ESLint config, the no-unkown-property rule drops out of the top 10 slowest rules entirely, with the 10th slowest clocking in at 180ms.

I think it would be a good idea to cache the React version when using detect instead of looking it up every time here: https://github.com/yannickcr/eslint-plugin-react/blob/ef9a51225e3d5f6cb7cbdc3ec0382ad49246034b/lib/util/version.js#L17-L33

Would you be open to a PR that did this?

@lencioni
Copy link
Collaborator Author

In the meantime, I'm going to use this bit of config:

  settings: {
    react: {
      // "detect" is slow
      // https://github.com/yannickcr/eslint-plugin-react/issues/2671
      // eslint-disable-next-line global-require
      version: require('react').version,
    },
  },

@ljharb
Copy link
Member

ljharb commented Jun 15, 2020

Caching it makes total sense.

ljharb pushed a commit that referenced this issue Jun 15, 2020
I've been timing and profiling ESLint runs at Airbnb and noticed that
react/no-unknown-property is particularly slow for us when using the
"detect" setting for React version.

When running ESLint using TIMING=1 on a directory that contains about
500 files to be linted in it, react/no-unknown-property shows up as
taking about 1700ms.

Looking at the callstacks in the profiler, it seems that when this rule
calls getStandardName for every JSXAttribute here, it will actually do
all of the work to detect the React version every time to determine the
correct set of DOM property names. Since there may be a lot of
JSXAttribute nodes in a codebase, this adds up to quite a bit of work.

By specifying the react version in our ESLint config, the
no-unkown-property rule drops out of the top 10 slowest rules entirely,
with the 10th slowest clocking in at 180ms.

I think it would be a good idea to cache the React version when using
detect instead of looking it up every time.

Fixes #2671.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants