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

Custom Map Function #42

Closed
lifehackett opened this issue Apr 12, 2016 · 5 comments
Closed

Custom Map Function #42

lifehackett opened this issue Apr 12, 2016 · 5 comments

Comments

@lifehackett
Copy link

I'm using https://github.com/rtfeldman/seamless-immutable lib for my data and it doesn't work with the native JS map function. It does work with libs like Ramda or lodash. I considered writing a wrapper component around <For> to extract my immutable structure into a JS array, but that would break some of the optimizations of immutable data.

Would you be open to a PR that either

  1. Uses one of these other lib map functions (which also happen to be more performant (https://jsperf.com/native-map-vs-lodash-map)
  2. Add a prop that takes a custom mapping function.

Thanks

@AlexGilleran
Copy link
Owner

I'm definitely not eager to introduce lodash given that we just got rid of it.

I guess a prop that takes a custom mapping function would be possible but if there's no other use cases it seems more like it should be up to seamless-immutable to fix rather than us (they have a bug for it here). Could you use the asMutable() method described there?

@texttechne
Copy link
Collaborator

It's more complicated to accomplish what you want than you imagine. Since jsx-control-statements is a babel plugin it just transforms <For> into map functions, so there is no code which can be swapped out by lodash`s map implementation. The relevant code is here.

I'm afraid that both approaches are not viable:

  1. Instead of transforming to myArray.map(...) we transform to _lodash(myArray, ...) (for example). But then lodash would become a peer dependency. This is quite unacceptable as @AlexGilleran pointed out. Also the implementation would be quite complicated, you would need to generate an import / require statement for every file where control statements occur, which is not as easy as it might sound.
  2. A custom mapping function sounds like a cool feature, but the only implementation I could imagine would work on the data model you put in (right now you put in an array and call map on it: myArray.map(...)). So it wouldn't work with lodash (_.map(immutableArray, ...))

I completely agree with @AlexGilleran: Use the asMutable() workaround for this bug.

@lifehackett
Copy link
Author

Thank you for your responses. I certainly understand not wanting to pull lodash back in...especially if you just removed it. I had seen it in the codebase when looking through it previously so thought it would be an option.

As for the "bug" in seamless-immutable, the author has weighed in on it and offered some good reasons for not doing it. The use case I am trying to solve is also not unique to seamless-immutable as the very popular ImmutableJS lib also would not work with native map.

As for asMutable(), unfortunately foo.asMutable() !== foo.asMutable() which would break certain key assumptions that my app operates under.

I didn't fully follow @texttechne 2nd point. I'll dig into the code to try and understand what you are saying. I have verified that a for loop would work with seamless-immutable if that is an option.

@texttechne
Copy link
Collaborator

Just tested it with Immutable.js and it works, since they provide a map function for collections.

Actually this is the point I'm talking about in 2). jsx-control-statements just calls map on the "array" / "collection" you provide. So jsx-control-statements does not by itself provide a specific implementation for map, it just expects that this function can be called on the given collection (it's like an interface or contract). Lodash uses a different interface, so just providing a mapping function via props won't suffice.

Unfortunately a for loop won't work either, since it doesn't make any sense in react:

<span>
  { 
    for(var i = 0; i < testArray.length; i++ {
      var item = testArray[i];
      ... // what to do now? you would need to return item...
    }
  }
</span>

Map functions do return something and are pure, for loops are only meant to achieve side-effects (to speak in FP) in their body.

By the way, we can always reopen the issue if you find an approach.

@AlexGilleran
Copy link
Owner

@lifehackett where are you using foo.asMutable() !== foo.asMutable()in this case? As I see it you're only creating a throwaway mutable view for foo in order to iterate over it - foo.asMutable()[0] should still === foo[0].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants