-
Notifications
You must be signed in to change notification settings - Fork 10
Enhancement to API for View: Return ViewRowData #4055
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
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.
Hi - started to read through, ended up editing/enhancing (I hope) a bunch of doc comments as I tried to understand this system again. Don't know that I quite understand it yet, but certainly didn't see any issues and am eager to adapt the small number of _meta usages I know of to the new API - seems like it will be a big improvement!
private _leafMap: Map<StoreRecordId, LeafRow> = null; | ||
private _recordMap: Map<StoreRecordId, StoreRecord> = null; | ||
_aggContext: AggregationContext = null; | ||
_rowCache: Map<string, BaseRow> = null; |
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.
This is a map of all rows (at all levels?) by their BaseRow.id
?
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.
This has been great to re-engage with our data package here.
The API updates seem like clear improvements - make me eager to add some more long-desired features, but I can hold back on my wishlist for future merges.
I would be happy for this to be merged at any point, but to be clear I haven't attempted to replicate or asses any findings around memory retention or the actual changes that prompted this work. And apols for blowing up the size of the changeset here with all of my comment updates - it was a good prompt to take another pass over all of that, didn't mean to hijack this PR so much. :)
@haynesjm42 leave it to you and Lee if there's anything else you want to suggest or test on this before merging and continuing the investigations.
# Conflicts: # CHANGELOG.md # data/cube/Cube.ts
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
develop
branch as of last change.breaking-change
label + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.