-
Notifications
You must be signed in to change notification settings - Fork 102
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
fix: properly handle all formats of options sources #963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked fine while testing, I have some minor comments though
export function useLabelCorrelation(options) { | ||
|
||
// This allows us to retrieve the label from a value in linear time | ||
const labelMap = useMemo(() => Object.assign({}, ...options.map((o) => ({ [_getValueHash(o.value)]: o.label }))), [ options ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have any performance issues? It's not a good idea to abuse useMemo
,useCallback
, etc because of memory leaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is why it's here and it's always been that way. I've just brought this pattern into a hook because we use it a few places. Otherwise the complexity is n^2 and we want the components to deal with values of n > 1000
* This hook allows us to retrieve the label from a value in linear time by caching it in a map | ||
* @param {Array} options | ||
*/ | ||
export function useLabelCorrelation(options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function useLabelCorrelation(options) { | |
export function useGetLabelCorrelation(options) { |
@@ -64,8 +66,7 @@ export default function Taglist(props) { | |||
onChange: props.onChange | |||
}); | |||
|
|||
// We cache a map of option values to their index so that we don't need to search the whole options array every time to correlate the label | |||
const valueToOptionMap = useMemo(() => Object.assign({}, ...options.map((o, x) => ({ [o.value]: options[x] }))), [ options ]); | |||
const correlateLabel = useLabelCorrelation(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const correlateLabel = useLabelCorrelation(options); | |
const getLabelCorrelation = useLabelCorrelation(options); |
@@ -19,6 +20,15 @@ export function sanitizeDateTimePickerValue(options) { | |||
return value; | |||
} | |||
|
|||
export function isDeepIncluded(value, array) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is not very descriptive, I think we can improve it
export function isDeepIncluded(value, array) { | |
export function hasEqualValue(value, array) { |
@@ -1,4 +1,5 @@ | |||
import { useRef } from 'preact/hooks'; | |||
import isDeepEqual from 'lodash/isEqual'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not rename it unless it's necessary
import isDeepEqual from 'lodash/isEqual'; | |
import isEqual from 'lodash/isEqual'; |
167dbc4
to
897cbf9
Compare
Closes #960