mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.55k stars 1.78k forks source link

Observable sets #69

Closed kmalakoff closed 5 years ago

kmalakoff commented 8 years ago

I have been using observable maps as sets, eg. myMap.set(key, true). Not an urgency, but if sets were provided in mobservable, I'd use them!

mweststrate commented 8 years ago

Closing this one for now, until more people need it.

mull commented 8 years ago

I just ran into this issue. I'd like to offer up my "need it". Seems like it doesn't work at the moment (though the milestone indicates it should.) Maybe I'm doing something else wrong.

leebenson commented 8 years ago

would love to see observable sets, too. I have a ton of components that rely on unique values. Currently, we're doing array look-ups (or the .set(val, true) trick @kmalakoff mentioned) which can get expensive when there's thousands of items. Sets solve this neatly.

gknaxtrader commented 7 years ago

I'd second that, thank you. @computed doesn't react on an observable someSet.clear() action.

mweststrate commented 7 years ago

I'll reopen it :) PR's are welcome ;-)

mweststrate commented 7 years ago

Closing. It seems we are pretty okay without, and having only a limited set of complex types (map, array, object) greatly simplifies libraries built on top of MobX

mull commented 7 years ago

Truth be told I feel non-okay without it. Using an observable Map and checking whether some key is === true ends up being very awkward, especially when new developers in the team join in and start using Mobx.

I wish I had the time to actually go ahead and implement it. Truth be told I had a look at doing it through Proxies but quite frankly couldn't navigate the source code well enough as I'm nearly oblivious to Typescript.

How do you feel about a bounty for implementing it? Hell even if it's a second npm module (until properly tested?) I'd be alright with that. See if we who request the feature will put our money where our mouth is!

matthewrobb commented 7 years ago

@mull Hey, I had this need recently and was able to get most of what I want doing this:

import {observable, ObservableMap } from 'mobx';
const { enhancer: deepEnhancer } = observable.map();

export class ObservableSet extends ObservableMap {
  constructor(initialValues=[], name) {
    super(initialValues.map(v => [v,v]), deepEnhancer, name);
  }

  static create() {
    return new this(...arguments);
  }

  add(value) {
    return this.set(value, value);
  }

  forEach(callback, thisArg = this) {
    for(const value of this) {
      callback.call(thisArg, value);
    }
  }

  [Symbol.iterator]() {
    return this.keys();
  }
}

You might need to override some of the added methods on ObservableMap to be Set-ish but this is all that's required to get base Set functionality to work 1:1 on top of ObservableMap.

natew commented 7 years ago

+1 on this, sets offer a nicer API for common patterns.

mweststrate commented 7 years ago

PR on mobx-utils?

Op wo 24 mei 2017 om 23:36 schreef Nate Wienert notifications@github.com:

+1 on this, sets offer a nicer API for common patterns.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/69#issuecomment-303858855, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhBK6LkESTeIHJJ-_9h1MCkoLiEClks5r9KL1gaJpZM4Gx4EB .

phpnode commented 7 years ago

@mweststrate it's small enough and a really useful feature, why not make it part of core?

Aldredcz commented 7 years ago

@mweststrate +1 on this. Gotta workaround with Map with true as value :( As I see it, one of the biggest benefits of MobX is that it mimics native JS objects seamlessly, and tha fact that Set is missing is really annoying.

phiresky commented 7 years ago

@observable mySet = new Set() just silently creates a ES6 Set, not triggering any observers for changes in the set. This is pretty confusing considering @observable myMap = new Map() works as expected.

It should at least print a warning, but then it could also just create a observable set<T> based on a observable map<T, null>

bradyep commented 7 years ago

A set is such a fundamental part of programming that I am surprised to see that they are not supported as observables in MobX, especially now that, as of ES6, they are now a standard part of JavaScript.

mweststrate commented 7 years ago

See the plans for MobX v4 as well. Sets are pretty limited when restricting yourself to ES5 (which MobX3 does), (e.g. only primitive keys are supported), and can be pretty easily expressed in userland by leveraging maps.

Op di 1 aug. 2017 om 21:33 schreef Brady Prigge notifications@github.com:

A set is such a fundamental part of programming that I am surprised to see that they are not supported as observables in MobX, especially now that, as of ES6, they are now a standard part of JavaScript.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/69#issuecomment-319473124, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhD-HN9Lt6BMdKEJpJh1SqulyWrUBks5sT31vgaJpZM4Gx4EB .

zfalen commented 7 years ago

+1 for this, Sets accomplish most of what simple list, etc. components need for rendering unique values and are just plain cleaner to work with than Maps or even Arrays in a lot of cases.

Plus, Sets just let you store and compare objects, which you can't do with the ObservableMap hack since keys have to be simple data types (Boolean, String, Number)

mweststrate commented 6 years ago

Sets and Maps won't be supported within the current major, as sets would be limited to strings, as Sets are not natively available on all ES5 complient JS runtimes and it goes well beyond the scope of this project to polyfill them

danielkcz commented 6 years ago

Polyfilling shouldn't be a concern for any library, you should just assume that Set is available and you are pretty much safe till user does @observable mySet = new Set(). At that point, it's clear that Set is available. So I find it a bit weak argument that you don't want to support it because of backward compatibility.

Can you please reconsider this? I am joining other people here as it's really awkward using Map for something simple as a unique list of items.

mweststrate commented 6 years ago

MobX 4 added support for real Maps, so if anybody wants to volunteer and add Sets in a similar fashion, PR's are welcome :). Please base the PR on mobx4 branch (see #1321)

Prior99 commented 6 years ago

I want to chime in and say "I need this". Especially for marking selections on Checkbox groups an there like Sets are almost a must-have.

mnpenner commented 6 years ago

I wasn't so keen on extending ObservableMap, so here's another approach:

import {observable, action} from 'mobx';

export default class ObservableSet {
    constructor(values) {
        this.map = values ? observable.map(values.map(v => [v, true])) : observable.map();
    }

    add(value) {
        this.map.set(value, true)
        return this;
    }

    delete(value) {
        return this.map.delete(value);
    }

    has(value) {
        return this.map.has(value);
    }

    @action.bound
    set(values) {
        this.clear();
        for(let v of values) {
            this.add(v);
        }
    }

    clear() {
        this.map.clear();
    }

    get size() {
        return this.map.size;
    }

    [Symbol.iterator]() {
        return this.map.keys();
    }
}

I've only been using it for a few minutes, but it seems to be working very well so far.

I added a bonus method set that will let you reset your Set without firing reactions multiple times.

It's still missing a few methods from ES6 Set... I haven't needed those yet.

jkeruzec commented 6 years ago

Maybe, an implementation with map is fine for MobX4 for IE11 compatibility, and new version of MobX 5 can have the real js Set implementation.

Interested in too !

stnwk commented 6 years ago

Would be great to have something like this! @mweststrate

mweststrate commented 5 years ago

Released in 4.9.0 / 5.9.0! (Note that your browser has to support Set to be able to use this feature)

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.