search

rtfeldman / seamless-immutable

Immutable data structures for JavaScript which are backwards-compatible with normal JS Arrays and Objects.
BSD 3-Clause "New" or "Revised" License
5.37k stars 195 forks source link

Issue#64 Add set and setIn operations (based on existing merge) #69

Closed haggholm closed 8 years ago

haggholm commented 8 years ago

Add simple implementation of set and setIn by constructing a path and invoking merge, which already does all the hard stuff, checking, managing mutability, &c.

haggholm commented 8 years ago

Refers to #64 (apparently the commit comment didn’t do the trick, sorry).

I’m not sure whether adding methods like this is really the right thing to do, but I followed to the model of merge. Perhaps it would be better to follow a lodash-like model of exported functions to avoid conflicts with named properties.

rtfeldman commented 8 years ago

Generally looks good! Sorry I took so long reviewing it.

I left a couple of comments, and there's also a merge conflict to resolve.

haggholm commented 8 years ago

Sorry I took even longer getting back! Added to README, changed a few random values to use JSC, not sure if that’s precisely what you meant: haven’t used JSC before (though if it’s inspired by QuickCheck, I expect to do so again).

johnste commented 8 years ago

Could this also be made to work with arrays as well? Currently it's cumbersome to update an item in an array.

haggholm commented 8 years ago

It can be done with arrays; I added it to arrays. I also found some weird edge cases where the object versions didn't work properly (with nested paths); I didn't nail it down, as my main concern was making the tests pass for my added methods, but wonkiness seemed to ensue sometimes when e.g. merging a complex object with a {foo: NaN} value. The new version is a bit more straightforward (IMO) and doesn't use merge(), so it's not affected, but it could use more digging. I can't take more company time for it right now, though!

rtfeldman commented 8 years ago

Thanks for this! :tada: