feat(property-definitions): make propertyDefintionModel lazy
#11454
Conversation
propertyDefintionModel lazypropertyDefintionModel lazy
|
|
||
| /** Update cached property definition metadata */ | ||
| export const updatePropertyDefinition = (propertyDefinitions: PropertyDefinition): void => { | ||
| propertyDefinitionsModel.findMounted()?.actions.updatePropertyDefinition(propertyDefinitions) |
There was a problem hiding this comment.
very nitty: this could call propertyDefinitionsModel.findMounted()?.actions.updatePropertyDefinitions([propertyDefinition]) and the reader doesn't have to look for the number of instances of the letter S to understand what is happening
There was a problem hiding this comment.
And then we remove actions.updatePropertyDefinition from the model and there's only one way of doing the thing
There was a problem hiding this comment.
Makes sense. I removed the singular action from everywhere now, so we must always pass an array.
| if (typeof propertyName === 'string' && !(propertyName in propertyDefinitionStorage)) { | ||
| window.setTimeout( | ||
| () => propertyDefinitionsModel.findMounted()?.actions.loadPropertyDefinitions([propertyName]), | ||
| 0 |
There was a problem hiding this comment.
Comment says 10ms code says 0. A mistake or arcane JS :)?
There was a problem hiding this comment.
Updated the comment. This line still gets a 0ms timeout to not e.g. execute when React is rendering. Once the action is called and there's something to fetch, we add a 10ms debounce.
| Object.fromEntries(pending.map((key) => [key, PropertyDefinitionState.Loading])) | ||
| ) | ||
| // and fetch them | ||
| const { url } = combineUrl('api/projects/@current/property_definitions/', { |
There was a problem hiding this comment.
should the URL combination be in api.ts instead of building it and calling api.get here?
There was a problem hiding this comment.
I think it should. Moved it!
| } | ||
|
|
||
| // nothing new to do | ||
| if (!mustLoad) { |
There was a problem hiding this comment.
Looks like this could just check Object.keys(pendingStateUpdate).length
There was a problem hiding this comment.
Makes sense and changed it. I used the bool as a form of premature optimization, as running Object.keys() does make a new array every time. Probably not worth the extra lines.
|
Thanks all! Feedback addressed (I hope), so merging. |
Problem
#8257
Changes
Converts
propertyDefinitionModelto a lazy-fetching operation, that fetches information about properties only when it needs to.How did you test this code?
WIP, so no tests yet.