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

Should `.map(...)` recursivily immutablize? #80

Closed nikki93 closed 8 years ago

nikki93 commented 8 years ago

I think the idea of map is map : (a -> b) -> T a -> T b, so maybe deeply immutablizing the results of map is taking it too far. Like the properties of functor T should be maintained, but nothing is known about b. I think it should just return an immutable array of whatever the function passed to it returns. I ran into this when, for example, mapping an immutable array to react elements like array.map((elem) => <Blah> ... </Blah>) which I feel like is a common use-case.

Wonder what you think. I guess though that if the property of T is that it's always deeply immutable then the library is good as-is. I think the use case of immutable arrays of mutable values is often encountered though!

Thanks for the great library!

rtfeldman commented 8 years ago

Hi @nikki93, we actually had this discussion awhile back; here are my thoughts: https://github.com/rtfeldman/seamless-immutable/issues/42#issuecomment-109204580

Thanks for raising this, but it's settled. :smile:

nikki93 commented 8 years ago

Thanks! I will say though that what I proposed is different: you do get back an Immutable, but not a deep one. So it is an Immutable array of objects that are mutable, or so. Basically what I was saying is in general map does T a -> T b which says nothing about b. The question then becomes 'can T contain mutable objects?'

rtfeldman commented 8 years ago

Ah. Not sure if I've expressed this elsewhere, but I'm not okay with T containing mutable objects. One of my goals for the library is for you to be able to rely on Immutable things being actually immutable, and allowing any mutable objects inside would break that. :smiley:

borisd commented 8 years ago

Perhaps its worth to add mapImmutable() as the current implementation prevents new comers from using the library as it breaks the regular React approach of:

todos.map(todo => <Todo todo={ todo } />