mobxjs / mobx-utils

Utility functions and common patterns for MobX
MIT License
1.19k stars 126 forks source link

Add ObservableGroupMap #245

Closed NaridaL closed 4 years ago

NaridaL commented 4 years ago

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 https://github.com/mobxjs/mobx/issues/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 commented 4 years ago

Coverage Status

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

NaridaL commented 4 years ago

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 commented 4 years ago

See docs here https://github.com/NaridaL/mobx-utils#observablegroupmap

mweststrate commented 4 years ago

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

NaridaL commented 4 years ago

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.