Improve and typescriptify update status #20546
Conversation
timroes
added
Feature:Visualizations
src/ui/public/vis/update_status.ts
Outdated
There was a problem hiding this comment.
This (maybe a bit weird looking syntax) will actually tell TypeScript, that the return value contains exactly those keys, that has been specified in the array passed to the first parameter (since that's where the T generic type is used).
That way TypeScript will actually fail on: getUpdateStatus([Status.TIME], {}, {}).data, since it knows the return type is of type { time: boolean; }.
💔 Build Failed |
|
jenkins, test this |
markov00
left a comment
markov00
left a comment
There was a problem hiding this comment.
Waiting for CI test before testing deeper. I've left few comments on the code since we are improving the update_status code I think they can be addressed here.
src/ui/public/vis/update_status.ts
Outdated
There was a problem hiding this comment.
What about moving this function out and create it as pure function?
Also the naming is a bit confusing, because seems that it just return true or false, but it also change a paramenter of the passed obj state. I rather prefer to have two separate function, one for checking and one for assign (if necessary).
Maybe the code will be a bit longer but you can get rid of any possible side effects of changing objects in the closure.
The function can be written like:
function hasChangedUsingGenericHashComparison(oldStatus: any, name: string, valueHash: any) {
const oldHash = oldStatus[name];
return oldHash !== valueHash;
}Than you just call it like
const valueHash = calculateObjectHash(param.visData);
status.data = hasChangedUsingGenericHashComparison(obj._oldStatus, 'data', valueHash);
obj._oldStatus.data = newHash;I found this more readable without knowing the internal of the calculate or hasChanged functions you know how object changes
src/ui/public/vis/update_status.ts
Outdated
There was a problem hiding this comment.
Same for the previous comment. Prefer pure functions and move them outside the current function scope.
If not possible, at least change the function name to reflect that it not only check the status but also update the oldStatus.
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
| height: number; | ||
| } | ||
|
|
||
| function hasSizeChanged(size: Size, oldSize?: Size): boolean { |
There was a problem hiding this comment.
Could you invert the params here so we are a bit more aligned with the previous: hasHashChanged function?
first the oldStatus and the new one?
There was a problem hiding this comment.
Unfortunately I can't because oldSize is optional so it must be in the end. If you want I can change the parameter order to the hasHashChanged function if you want to hash, oldStatus, key if you think that increases readability.
There was a problem hiding this comment.
oo you are right, sorry. Yes please so we have in the switch part a cleaner readibility,
thanks ❤️
💚 Build Succeeded |
|
Reverted the tslint changes out of it, since it seems that PR is still not merged. @ppisljar care for a review? |
💚 Build Succeeded |
| status.resize = hasSizeChanged(size, obj._oldStatus.resize); | ||
| obj._oldStatus.resize = size; | ||
| break; | ||
| case Status.TIME: |
There was a problem hiding this comment.
Cool. I think we should remove the TIME parameter itself in a separate PR< since we need to change developer docs with that one.
4 participants
This PR does a couple of improvements and fixes at the update status calculation:
TIMEstatus would not be calculated correctly anymore, since the parameter onvismoved to a different location from earlier.data, so we can get rid of the slowJSON.stringifyin this place.