Skip to content
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

Add ObservableGroupMap #245

Merged
merged 5 commits into from
May 15, 2020
Merged

Add ObservableGroupMap #245

merged 5 commits into from
May 15, 2020

Conversation

NaridaL
Copy link
Collaborator

@NaridaL NaridaL commented Apr 4, 2020

An ObservableGroupMap is constructed from a base IObservableArray and a groupBy function.
It maps groupBy values to IObservableArrays of the items in the base array which have groupBy(item) == key
The IObservableArrays are updated reactively when items in the base array are removed, changed or added.
Updates are done without iterating through the base array (except once at the beginning)

This is a first draft to demonstrate the idea, if there's interest in adding it to mobx-utils, I will make it more robust and add proper tests.

My concrete usecase is basically what I've implemented at the bottom of the file:

If I have an array of time-intervals, and I want to calculate aggregate statistics about them (for example sums per day) the obvious solution has terrible performance:

const intervalsByDay = getAllDays(intervals).map(day => intervals.filter(i => isSameDay(i, day))

... which has O(n²)

I can improve upon that by iterating once and building a map, and then I can improve even more, by only adjusting the output arrays when actually necessary, which is what this PR does.

I went through the docs carefully and didn't find anything for this usecase.

This partly covers mobxjs/mobx#166

Open issues:

Use plain Map or ObservableMap as base Map? (or configurable) ObservableMap as base makes most sense, because then ogm.keys() is observable, as well as for example (ogm.get(x) ?? []).length to observe the number of items with a particular groupBy

[ ] listen to onBecomeObserved /onBecomeUnobserved events or make dispose explicit? If the group-map only becomes unobserved for a short while, the overhead of disposing of all the listeners and recreating them could be significant.

@coveralls
Copy link

coveralls commented Apr 4, 2020

Coverage Status

Coverage decreased (-1.5%) to 91.988% when pulling c13110f on NaridaL:master into 4b59640 on mobxjs:master.

@NaridaL
Copy link
Collaborator Author

NaridaL commented Apr 4, 2020

Logged output of the current "test":

[mobx.trace] 'Autorun@9' tracing enabled
{"mo":[{"day":"mo","hours":3},{"day":"mo","hours":3}],"tu":[{"day":"tu","hours":3}],"we":[{"day":"we","hours":3}]}
[mobx.trace] 'moHours' tracing enabled
moHours 6
[mobx.trace] 'tuHours' tracing enabled
tuHours 3
getDependencyTree {
    "name": "Autorun@9",
    "dependencies": [
        {
            "name": "moHours",
            "dependencies": [
                {
                    "name": "GroupArray[mo]"
                },
                {
                    "name": "base[..].hours"
                },
                {
                    "name": "base[..].hours"
                }
            ]
        },
        {
            "name": "tuHours",
            "dependencies": [
                {
                    "name": "GroupArray[tu]"
                },
                {
                    "name": "base[..].hours"
                }
            ]
        }
    ]
}
>> add tu 4
[mobx.trace] 'tuHours' is invalidated due to a change in: 'GroupArray[tu]'
[mobx.trace] 'Autorun@9' is invalidated due to a change in: 'tuHours'
{"mo":[{"day":"mo","hours":3},{"day":"mo","hours":3}],"tu":[{"day":"tu","hours":3},{"day":"tu","hours":4}],"we":[{"day":"we","hours":3}]}
moHours 6
tuHours 7
>> remove we slice
>> set first mo slice hours
[mobx.trace] 'moHours' is invalidated due to a change in: 'base[..].hours'
[mobx.trace] 'Autorun@9' is invalidated due to a change in: 'moHours'
{"mo":[{"day":"mo","hours":12},{"day":"mo","hours":3}],"tu":[{"day":"tu","hours":3},{"day":"tu","hours":4}],"we":[]}
moHours 15
tuHours 7
>> replace tu slice
[mobx.trace] 'tuHours' is invalidated due to a change in: 'GroupArray[tu]'
[mobx.trace] 'Autorun@9' is invalidated due to a change in: 'tuHours'
{"mo":[{"day":"mo","hours":12},{"day":"mo","hours":3}],"tu":[{"day":"tu","hours":4},{"day":"tu","hours":4.5}],"we":[]}
moHours 15
tuHours 8.5

@NaridaL NaridaL marked this pull request as draft April 10, 2020 12:24
@NaridaL NaridaL marked this pull request as ready for review April 10, 2020 12:58
@NaridaL NaridaL force-pushed the master branch 3 times, most recently from 1aba0df to eab2810 Compare April 10, 2020 13:15
@NaridaL
Copy link
Collaborator Author

NaridaL commented Apr 10, 2020

@NaridaL NaridaL changed the title WIP: Add ObservableGroupMap Add ObservableGroupMap Apr 10, 2020
@mweststrate
Copy link
Member

Looks cool! I'd take ObservableMap as basis, and make disposal explicit indeed :) For example through intervalsByDay.dispose()

An ObservableGroupMap is constructed from a base IObservableArray and a `groupBy` function.
It maps `groupBy` values to IObservableArrays of the items in the base array which have groupBy(item) == key
The IObservableArrays are updated reactively when items in the base array are removed, changed or added.
Updates are done without iterating through the base array (except once at the beginning)
@NaridaL
Copy link
Collaborator Author

NaridaL commented May 5, 2020

Yeah, ObservableMap makes sense, because then you can observe for example intervalsByDay.get("su"), even if it doesn't exist yet.

dispose is also what makes most sense to me.

I've added some more tests and improved the documentation, I'll merge this in a couple of days if no one objects.

@NaridaL NaridaL merged commit dbd3975 into mobxjs:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants