[perf] cache resolved data element options#6579
Conversation
src/helpers/helpers.options.js
Outdated
| * @param {object} [context] - If defined and the current value is a function, the value | ||
| * is called with `context` as first argument and the result becomes the new input. | ||
| * @param {number} [index] - If defined and the current value is an array, the value | ||
| * @param {object} [info] - object to return information about resolution in |
There was a problem hiding this comment.
I think I'd initialize info.cacheable to true in this method so that if you call resolve outside of _resolveDataElementOptions then info contains a usable value. Then in _resolveDataElementOptions you can check if (info.cacheable && !custom) instead of if (info.cacheable)
There was a problem hiding this comment.
It would have to be something like
if (info && info.cacheable === undefined) {
info.cacheable = true;
}because resolve is called in a loop. But I think its not needed. You'd need to provide the info object anyway, so not a biggie to provide initial value too.
There was a problem hiding this comment.
In other words, I like to keep the initialization (and any related if's) out of loop
There was a problem hiding this comment.
Maybe just document it then?
* @param {boolean} [info.cacheable] - Should be initialized to true. Will be updated by this method.
benmccann
left a comment
benmccann
left a comment
There was a problem hiding this comment.
This looks good! Definitely less code than my version!
I had opened my PR previously and you left a comment that I didn't understand. I was going to follow up with you to clarify, but now I understand :-) Thanks so much!
|
Was about to merge but I thought of this case:
Or am I missing something here? |
|
DatasetController does |
|
Right, just wanted to make sure that case was tested |
* [perf] cache resolved data element options * Address review comments * Move uninitialized variables, update comments
3 participants
Alternative approach that came to mind while taking a look at #6574
Closes #6382