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.

Sign up for GitHub

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(customPropTypes): improve perf of suggest #2314

Merged
merged 1 commit into from
Nov 25, 2017
Merged

fix(customPropTypes): improve perf of suggest #2314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 61 additions & 44 deletions src/lib/customPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,63 +12,80 @@ export const as = (...args) => PropTypes.oneOfType([
PropTypes.func,
])(...args)

/* eslint-disable max-nested-callbacks */
const findBestSuggestions = _.memoize((propValueWords, suggestions) => _.flow(
_.map((suggestion) => {
const suggestionWords = suggestion.split(' ')

const propValueScore = _.flow(
_.map(x => _.map(y => leven(x, y), suggestionWords)),
_.map(_.min),
_.sum,
)(propValueWords)

const suggestionScore = _.flow(
_.map(x => _.map(y => leven(x, y), propValueWords)),
_.map(_.min),
_.sum,
)(suggestionWords)

return { suggestion, score: propValueScore + suggestionScore }
}),
_.sortBy(['score', 'suggestion']),
_.take(3),
)(suggestions))
/* eslint-enable max-nested-callbacks */

/**
* Similar to PropTypes.oneOf but shows closest matches.
* Word order is ignored allowing `left chevron` to match `chevron left`.
* Useful for very large lists of options (e.g. Icon name, Flag name, etc.)
* @param {string[]} suggestions An array of allowed values.
*/
export const suggest = suggestions => (props, propName, componentName) => {
export const suggest = (suggestions) => {
if (!Array.isArray(suggestions)) {
throw new Error([
'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.')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
const propValue = props[propName]

// 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) => {
Copy link
Member Author

@dvdzkwsk dvdzkwsk Nov 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) // 2

Note 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.

const propValueWords = str.split(' ')

return _.flow(
_.map((suggestion) => {
const suggestionWords = suggestion.split(' ')

const propValueScore = _.flow(
_.map(x => _.map(y => leven(x, y), suggestionWords)),
_.map(_.min),
_.sum,
)(propValueWords)

const suggestionScore = _.flow(
_.map(x => _.map(y => leven(x, y), propValueWords)),
_.map(_.min),
_.sum,
)(suggestionWords)

return { suggestion, score: propValueScore + suggestionScore }
}),
_.sortBy(['score', 'suggestion']),
_.take(3),
)(suggestions)
})
/* eslint-enable max-nested-callbacks */

// Convert the suggestions list into a hash map for O(n) lookup times. Ensure
// the words in each key are sorted alphabetically so that we have a consistent
// 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key sorting

return acc
}, {})

return (props, propName, componentName) => {
const propValue = props[propName]

// skip if prop is undefined or is included in the suggestions
if (!propValue || suggestionsLookup[propValue]) return

// check if the words were correct but ordered differently.
// Since we're matching for classNames we need to allow any word order
// to pass validation, e.g. `left chevron` vs `chevron left`.
const propValueSorted = propValue.split(' ').sort().join(' ')
if (suggestionsLookup[propValueSorted]) return

// find best suggestions
const propValueWords = propValue.split(' ')
const bestMatches = findBestSuggestions(propValueWords, suggestions)
// find best suggestions
const bestMatches = findBestSuggestions(propValue)

// skip if a match scored 0
// since we're matching on words (classNames) this allows any word order to pass validation
// e.g. `left chevron` vs `chevron left`
if (bestMatches.some(x => x.score === 0)) return
// skip if a match scored 0
if (bestMatches.some(x => x.score === 0)) return

return new Error([
`Invalid prop \`${propName}\` of value \`${propValue}\` supplied to \`${componentName}\`.`,
`\n\nInstead of \`${propValue}\`, did you mean:`,
bestMatches.map(x => `\n - ${x.suggestion}`).join(''),
'\n',
].join(''))
return new Error([
`Invalid prop \`${propName}\` of value \`${propValue}\` supplied to \`${componentName}\`.`,
`\n\nInstead of \`${propValue}\`, did you mean:`,
bestMatches.map(x => `\n - ${x.suggestion}`).join(''),
'\n',
].join(''))
}
}

/**
Expand Down
3 changes: 1 addition & 2 deletions test/specs/lib/customPropTypes-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import { customPropTypes } from 'src/lib'

describe('suggest prop type', () => {
it('should throw error when non-array argument given', () => {
const propType = customPropTypes.suggest('foo')
expect(() => propType({ name: 'bar' }, 'name', 'FooComponent')).to.throw(
expect(() => customPropTypes.suggest('foo')).to.throw(
Error,
/Invalid argument supplied to suggest, expected an instance of array./,
)
Expand Down