feat: add way to get all instances of a type of model from dashboard#649
feat: add way to get all instances of a type of model from dashboard#649jake-bassett wants to merge 4 commits intomainfrom
Conversation
jake-bassett
changed the title
Codecov Report
@@ Coverage Diff @@
## main #649 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 53 53
Lines 1452 1462 +10
Branches 202 205 +3
=========================================
+ Hits 1452 1462 +10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/model/manager/model-manager.ts
Outdated
| */ | ||
| export class ModelManager { | ||
| private readonly modelInstanceMap: WeakMap<object, ModelInstanceData> = new WeakMap(); | ||
| private readonly modelInstanceMap: Map<object, ModelInstanceData> = new Map(); |
There was a problem hiding this comment.
Highlighting this change so others see it in the review.
There was a problem hiding this comment.
Switching this to a strongly referenced map means we'd need to be more careful on memory leaks and manually manage this.
aaron-steinfeld
left a comment
aaron-steinfeld
left a comment
There was a problem hiding this comment.
Left some impl comments, but potentially moot based on first comment.
| * Returns a shallow copy array of model instances within a dashboard that match the | ||
| * argument model class | ||
| */ | ||
| getModelInstances<T extends object>(modelClass: Constructable<T>): object[]; |
There was a problem hiding this comment.
I thought your use case was to instantiate a model without a renderer? If that's the case, no changes are needed in this repo, just in hyperdash-angular to export certain APIs.
There was a problem hiding this comment.
(i,e. model manager's programmatic instantiation APIs)
src/model/manager/model-manager.ts
Outdated
| */ | ||
| export class ModelManager { | ||
| private readonly modelInstanceMap: WeakMap<object, ModelInstanceData> = new WeakMap(); | ||
| private readonly modelInstanceMap: Map<object, ModelInstanceData> = new Map(); |
There was a problem hiding this comment.
Switching this to a strongly referenced map means we'd need to be more careful on memory leaks and manually manage this.
src/model/manager/model-manager.ts
Outdated
| * Returns a shallow copy array of model instances that match the argument model class | ||
| */ | ||
| public getModelInstances<T extends object>(modelClass: Constructable<T>): object[] { | ||
| return Array.from(this.modelInstanceMap.keys()).filter(modelInstance => modelInstance instanceof modelClass); |
There was a problem hiding this comment.
This doesn't match the behavior you're looking for. The model instance map in this singleton is all models, you're using this from the context of one dashboard. Instead, you'd want to pass in the root from that dashboard and then use get children to walk that model's tree.
src/model/manager/model-manager.ts
Outdated
| */ | ||
| public getModelInstances<T extends object>(modelClass: Constructable<T>): object[] { | ||
| return Array.from(this.modelInstanceMap.keys()).filter(modelInstance => modelInstance instanceof modelClass); | ||
| public getModelInstances<T extends object>(modelClass: Constructable<T>, root?: object): object[] { |
There was a problem hiding this comment.
we'd never want to call this without root, and once you have root, you don't need to enumerate keys so you can revert the instancemap back to a weakmap.
2 participants
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.