fix(customPropTypes): improve perf of suggest
#2314
Conversation
| 'Invalid argument supplied to suggest, expected an instance of array.', | ||
| ` See \`${propName}\` prop in \`${componentName}\`.`, | ||
| ].join('')) | ||
| throw new Error('Invalid argument supplied to suggest, expected an instance of array.') |
There was a problem hiding this comment.
We don't need to perform this isArray check every time the component is checked, since it's only used to initially define prop types. Given my current change we lose some information about which property was specified incorrectly, but the benefit is that if it is used improperly it will throw immediately.
src/lib/customPropTypes.js
Outdated
| // skip if prop is undefined or is included in the suggestions | ||
| if (_.isNil(propValue) || propValue === false || _.includes(propValue, suggestions)) return | ||
| // Convert the suggestions list into | ||
| const suggestionsLookup = suggestions.reduce((acc, key) => { |
There was a problem hiding this comment.
Convert O(n) lookup to O(1).
dvdzkwsk
force-pushed
the
chore/suggest-perf
branch
2 times, most recently
from
5711d29 to
ee79375
Compare
Codecov Report
@@ Coverage Diff @@
## master #2314 +/- ##
=======================================
Coverage 99.73% 99.73%
=======================================
Files 152 152
Lines 2655 2655
=======================================
Hits 2648 2648
Misses 7 7Continue to review full report at Codecov.
|
dvdzkwsk
force-pushed
the
chore/suggest-perf
branch
from
ee79375 to
27c3dcc
Compare
dvdzkwsk
force-pushed
the
chore/suggest-perf
branch
from
27c3dcc to
85d36f0
Compare
| // skip if prop is undefined or is included in the suggestions | ||
| if (_.isNil(propValue) || propValue === false || _.includes(propValue, suggestions)) return | ||
| /* eslint-disable max-nested-callbacks */ | ||
| const findBestSuggestions = _.memoize((str) => { |
There was a problem hiding this comment.
Implementation is the same, just passed the base string in instead of an array so that _.memoize can actually do its job, since arrays are not memoized by value:
let calls = 0
const fn = _.memoize((arr) => ++calls)
fn(['my', 'words'])
fn(['my', 'words'])
console.log(calls) // 2Note that the previous usage of this memoized function was incorrect, since it would ignore the second argument suggestions, meaning the first call to it with a given propValue would have won out. This was unintentionally mitigated by the fact that _.memoize only tracks the first argument by default, and that argument was an array, ergo no actual memoization was occurring.
The real fix would be to write our own resolver function for the memoization, but it would essentially achieve the same thing as I'm doing here by inlining the function, given the use case for this suggest utility.
| // way of looking up a value in the map, i.e. we can sort the words in the | ||
| // incoming propValue and look that up without having to check all permutations. | ||
| const suggestionsLookup = suggestions.reduce((acc, key) => { | ||
| acc[key.split(' ').sort().join(' ')] = true |
|
Anything else needed here? |
|
@davezuko I want to hear @levithomason before merge this. Let's wait for his review. |
|
I had a few conversations with @davezuko over Slack about this. It should make a pretty big improvement. Actual metrics would be nice, but I don't have time for it. Will release soon. |
|
Released in |
This is not ready yet because it's not solving the case presented by #2301, which concerns word ordering.Word ordering has been solved by way of key sorting.