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

Support for Set and Map #146

Closed
cmmartin opened this issue Apr 27, 2018 · 20 comments
Labels

Comments

@cmmartin
Copy link

@cmmartin cmmartin commented Apr 27, 2018

  • Feature request: Is it possible to support mutating the native Set, Map, WeakMap, etc?
    • I am willing to implement this myself
@RikuVan

This comment has been minimized.

Copy link

@RikuVan RikuVan commented Apr 29, 2018

I had a quick try at adding Set and Map for es6 environment. I don't completely get everything happening in immer, so I well may have missed some complexities. At first glance it seems actually pretty easy to proxy these since all mutations happen through method calls:

const nonMutatingMethods = [
    "forEach",
    "get",
    "has",
    "keys",
    "set",
    "values",
    "entries",
    "@@iterator"
]

const mutatingMethods = ["set", "delete", "clear", "add"]

const properties = ["size"]

function source(state) {
    return state.modified ? state.copy : state.base
}

export function proxyMapOrSet(state) {
    var proxyState = state
    var proxy = {}
    nonMutatingMethods.forEach(function(method) {
        proxy[method] = function(...args) {
            return source(proxyState)[method](...args)
        }
    })
    properties.forEach(function(prop) {
        proxy[prop] = source(proxyState)[prop]
    })
    mutatingMethods.forEach(function(method) {
        proxy[method] = function(...args) {
            markChanged(proxyState)
            return proxyState.copy[method](...args)
        }
    })
    createHiddenProperty(proxy, PROXY_STATE, proxyState)
    return {proxy}
}

I guess you don't need to recurse into the Map/Set values since they are all set through method calls?

@RikuVan

This comment has been minimized.

Copy link

@RikuVan RikuVan commented Apr 29, 2018

but it seems you can't freeze these, which means you have to do something kind of hacky there I guess:

function freezeMapOrSet(value) {
    const insertMethod = isMap(value) ? "set" : "add"
    if (autoFreeze) {
        ;[insertMethod, "delete", "clear"].forEach(function(method) {
            Object.defineProperty(value, method, {
                value: function() {
                    throw "In a frozen state, cannot be mutated"
                },
                enumerable: false,
                writable: false
            })
        })
        Object.freeze(value)
    }
    return value
}
@RikuVan

This comment has been minimized.

Copy link

@RikuVan RikuVan commented May 1, 2018

Tried it out today here https://github.com/RikuVan/immer/tree/support_map_and_set with only a few basic tests

@bradennapier

This comment has been minimized.

Copy link

@bradennapier bradennapier commented Jun 5, 2018

So I wanted to get full support for ES6 Map/Set. I also needed to get change logs of every update -- since the proxy makes this pretty efficient I took my own stab at re-building the library a bit.

immuta

It is still a little messy since I was just getting it into the fold but the example does a good example of showing it in action:

import immuta from "immuta";
import printDifference from "immuta/utils/print-difference";

const state = {
  deep: {
    foo: {
      bar: {
        baz: true
      }
    },
    set: new Set([{ one: "two" }, { two: "three" }]),
    map: new Map([["one", { foo: "bar" }]]),
    array: [{ i: 1 }, { i: 2 }, { i: 3 }, { i: 4 }, { i: 5 }]
  }
};

const next = immuta(
  // provide the state to start with
  state,
  // draft is a proxy that will copy-on-write
  draft => {
    const one = draft.deep.map.get("one");
    if (one) {
      one.foo = 1;
    }
    draft.deep.set.clear();
    draft.deep.set.add({ some: "obj" });

    draft.deep.array[2].foo = "bar!";
  },
  // optional callback for change events
  (changedState, changedMap, rollback) => {
    // rollback() will cancel the changes and return original object
    // changedMap is Map { 'path.to.change' => changedValue }
    // changedState is the new state being returned to caller (nextState)
    changedMap.forEach((v, changedKey) => {
      console.log("Change: ", changedKey);
    });
  }
);

printDifference(state, next);
Change:  deep
Change:  deep.map(=> [MAP])
Change:  deep.map(=> [MAP]).get.(one)
Change:  deep.map(=> [MAP]).get.(one).foo
Change:  deep.set(=> [SET])
Change:  deep.array
Change:  deep.array.2
Change:  deep.array.2.foo

image

As for the Object.freeze - you can't freeze it directly using Object.freeze() but you could certainly use a proxy to do it. Not quite there yet.

Probably the most annoying part is having to handle stepping multiple times when receiving functions (to determine if the function is supposed to belong to our Map/Set). Question also becomes whether or not the support should instead be "Set-like and Map-like instances" rather than requiring it be a true instanceof

For me I also needed to add the extra syntax and RE which will be a perf hit - plan to rethink how that is implemented shortly.

It should go as deep as needed. It will obviously not, and never should, support Weak anything.

Although I've done some thinking about what it would look like to take this to the next level and support function calls with given arguments (since Set/Map required the creation of such a concept). This would be kind of insane/interesting but probably a bad idea/for another lib :-P


Hoping my efforts here might be of some use in some way to y'all! Thanks for the concept, it's solid!


Update - Just to update - got one last blocker in place but believe im able to rid most of the hurdles here using this format and it should translate well into immer

@RikuVan

This comment has been minimized.

Copy link

@RikuVan RikuVan commented Jun 15, 2018

Cool, I am curious to look at your version more carefully as soon as I have more time.

@bradennapier

This comment has been minimized.

Copy link

@bradennapier bradennapier commented Jun 17, 2018

Yeah it is working well now and performs very well. As for mentioning not needing to recurse, I disagree. Take this example:

const state = {
  deep: {
    foo: {
    map: new Map([['one', { foo: 'bar' }]]),
  },
};

const next = immuta(
  // provide the state to start with
  state,
  // draft is a proxy that will copy-on-write
  draft => {
    const val = draft.deep.map.get('one');
    val.baz = 'qux';
  },
  // optional callback for change events
  (changedState, changedMap, rollback) => {
    // rollback() will cancel the changes and return original object
    // changedMap is Map { 'path.to.change' => changedValue }
    // changedState is the new state being returned to caller (nextState)
    changedMap.forEach((v, changedKey) => {
      console.log('Change: ', changedKey);
    });
  },
);

Which I need to improve still but it works for all cases I've tested against thus far. Just would be issue with non-confirming prototype methods so need to do an instance check

immuta implementation

@bradennapier

This comment has been minimized.

Copy link

@bradennapier bradennapier commented Jul 25, 2018

I provided some more insight into some of the challenges with Set (and a little bit into Map) with the release of some extra helpers to handle deep merging. They may also be helpful to anyone who might want to take on such a task.

odo-network/immuta#1

Essentially Set introduces a fairly difficult problem in that it when you want to change the reference to the item in the Set you actually need to delete the original value which ends up changing the insertion order of the Set.

Merge is essentially impossible because of this, when insertion order is no longer maintained there is really no way to "map" the desired values to their originals that we want to merge with.

Overall Map is pretty perfect (aside from keys not being immutable), but Set is fair to say is almost guaranteed to cause problems when merging - and possible to cause issues when doing standard mutations in a draft (using iteration) -- which would be fairly slow anyway since it cant use mergeWithDraft and must iterate the proxy.

@mweststrate

This comment has been minimized.

Copy link
Collaborator

@mweststrate mweststrate commented Aug 16, 2018

For now added some docs on how to work with Map's and Set's in combination with Immer:

d9b4d56

Closing this issue for now, but if PR shows up that supports both Maps and Sets completely, with deep updates and also works in ES5 mode :)

@szagi3891

This comment has been minimized.

Copy link

@szagi3891 szagi3891 commented Sep 16, 2018

Someone might want to add this PR? :)

Support for maps and set is the last thing that is missing in this cool library. Then I could get rid of the dependence on immutable.js

@aigoncharov

This comment has been minimized.

Copy link
Contributor

@aigoncharov aigoncharov commented Apr 14, 2019

@mweststrate what if a PR for ES6 only is going to show up? Moreover, what if it is only for Maps?
Let me be honest, I use Maps a lot and I really really would like immer to have a first-class support for them. Yet, I, fortunately, do not have to support ES5 envs in my projects. So I'm wondering if it makes sense for me to work on a PR for immer which includes at least (but not at most) ES6 support for Maps only?
My priorities would be:

  1. (Required minimum) ES6 Maps support
  2. (Desireable, but not guaranteed) ES6 Sets support
  3. (Optional, would work on it only if it doesn't take too much of my time) ES5 Maps support
  4. (Optional, would work on it only if it doesn't take too much of my time)ES5 Sets support

Would it be beneficial for the community to have at least ES6 Maps support? Would you be willing to merge such a PR?

@aigoncharov

This comment has been minimized.

Copy link
Contributor

@aigoncharov aigoncharov commented Apr 14, 2019

@mweststrate I also have a question on the design. Would you expect a mutation of a Map item to cause a creation of a new Map?

In other words, should this work?

const state = {
   users: new Map([["michel", {name: "miche"}]])
}

const nextState = produce(state, draft => {
   const user = draft.users.get("michel")
   // Should this one work, effectively creating a new Map?
   user.name = 'Aladdin'
})
@mweststrate

This comment has been minimized.

Copy link
Collaborator

@mweststrate mweststrate commented Apr 15, 2019

@aigoncharov

This comment has been minimized.

Copy link
Contributor

@aigoncharov aigoncharov commented Apr 15, 2019

So far I managed to make an early preview of nested updates for ES6 Maps. The ongoing work can be tracked here. If anyone is interested in providing any kind of help with this feature please email me at andrey.goncharov+it@protonmail.com

@aigoncharov

This comment has been minimized.

Copy link
Contributor

@aigoncharov aigoncharov commented Apr 17, 2019

@mweststrate PR for ES6 maps #350

@aigoncharov aigoncharov referenced this issue Apr 18, 2019
13 of 13 tasks complete
@mweststrate

This comment has been minimized.

Copy link
Collaborator

@mweststrate mweststrate commented Sep 13, 2019

Reopened this issue, as the PR is still pending,

@mweststrate mweststrate reopened this Sep 13, 2019
@mweststrate mweststrate added the has PR label Sep 13, 2019
@runnez

This comment has been minimized.

Copy link
Collaborator

@runnez runnez commented Oct 5, 2019

@mweststrate @aleclarson Guys look at this #437

@vytautas-pranskunas-

This comment has been minimized.

Copy link

@vytautas-pranskunas- vytautas-pranskunas- commented Oct 10, 2019

any updates on this?

@mweststrate

This comment has been minimized.

Copy link
Collaborator

@mweststrate mweststrate commented Oct 10, 2019

If the PR is solid, it's coming, but didn't have any time to review yet.

@vytautas-pranskunas-

This comment has been minimized.

Copy link

@vytautas-pranskunas- vytautas-pranskunas- commented Oct 11, 2019

@mweststrate

This comment has been minimized.

Copy link
Collaborator

@mweststrate mweststrate commented Oct 31, 2019

This has been released as Immer 5.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.