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

Optimize Icon validation in development (and others?) #1184

Closed
dvdzkwsk opened this issue Jan 19, 2017 · 5 comments
Closed

Optimize Icon validation in development (and others?) #1184

dvdzkwsk opened this issue Jan 19, 2017 · 5 comments

Comments

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Jan 19, 2017

This should be a fairly easy fix, but it is quite important as I noticed non-negligible slowdown in one of our applications today until implementing local workarounds. Steps below, posting this as a reminder so we don't forget about it.

Steps

  1. Render the Icon component.
  2. Note the render time, using something like the React perf tools
  3. For extra fun, run a bunch of repeated renders fairly quickly, as in a normal app environment

Expected Result

It should be super fast, the icon is a tiny component!

Actual Result

It can cripple applications in development when propTypes are not ignored due to expensive validation on supported icon names.

Version

v0.64.2

Testcase

Not yet provided

@levithomason
Copy link
Member

levithomason commented Jan 19, 2017

The suggest prop type checker is expensive, especially on massive lists of options like icon names and flag names. Simply _.memoize()ing it would do wonders here.

@levithomason
Copy link
Member

levithomason commented Apr 7, 2017

Until this is solved, @traverse suggested a great workaround in Gitter about this. You can skip prop validation entirely by simply using className instead of name. In this component's case, the name is forwarded directly to the className without modification, only validation.

<Icon name='user' />        // will validate, is slow
<Icon className='user' />   // no validation, is fast

Also note, propTypes are stripped in production automatically so this should only be a development issue.

@rahilsondhi
Copy link

The performance of my app improved significantly when changing name to className. Redux actions that used to take 300ms now take under 50ms (this is all in development). We should make this more clear to users that they shouldn't use name, or we should just remove suggest.

@levithomason
Copy link
Member

I don't think using name is a good solution, so it is not promoted beyond this issue as a workaround. I'd like to keep the validation as it is super useful.

Memoizing is the answer here. It is pretty minimal in this case. There should be a cache of input values and suggested results. After the first render, this would be nearly as fast as using className only.

PRs welcome.

@ayastreb
Copy link
Contributor

ayastreb commented Oct 1, 2017

I'd like to take this issue, if noone has started working on it yet.

ayastreb added a commit to ayastreb/Semantic-UI-React that referenced this issue Oct 2, 2017
cache finding suggestions for prop words
ayastreb added a commit to ayastreb/Semantic-UI-React that referenced this issue Oct 2, 2017
cache finding suggestions for prop words
levithomason pushed a commit that referenced this issue Oct 20, 2017
cache finding suggestions for prop words
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants