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

Add support for Immutable.js #23

Open
petrbrzek opened this issue Jul 15, 2016 · 11 comments
Open

Add support for Immutable.js #23

petrbrzek opened this issue Jul 15, 2016 · 11 comments

Comments

@petrbrzek
Copy link

It would be really nice to seamlessly use this library with Immutable.js structures. Now we must call .toJS() and that could be a bottleneck.

@jeancroy
Copy link
Owner

Ok what kind of structure do you have ?
Array of string ? Array of object on which you query a key ?

I've never worked with immutable.js but we are 100% read only so I'm confident we can.
Does it have a special way to detect collection is immutable and iterate on them ?

@jeancroy
Copy link
Owner

I'm thinking of providing a callback that could be used in a map like function.

var cb = fz.callBackFor(query,option)
ImmutableCollectionOfCandidate.map( cb ).filter( .. ).sort( .. )

Altougth sorted probably need mutability

@petrbrzek
Copy link
Author

petrbrzek commented Jul 15, 2016

@jeancroy we have a list of maps (array of plain objects in pure JS). Immutable.js has sort method so that's not a problem. The only difference between array of plain objects and Immutable structures for this library are getters. For plain objects you use the dot sign object.name for accessing properties but in Immutable you have to use .get('name').

Maybe we could pass some flag so you would know the structure is Immutable.

@jeancroy
Copy link
Owner

This is how I iterate over the list and extract the key to be ranked
https://github.com/jeancroy/fuzzaldrin-plus/blob/master/src/filter.coffee#L16-L17

I could provide a "getterFunc" in the optionHash that input the element and output the value to be scored. Would that make sens, flexibility and performance wise ?

@petrbrzek
Copy link
Author

I will try to hack it and let you know if that's enough or something else needs to be changed.

@petrbrzek
Copy link
Author

petrbrzek commented Jul 18, 2016

Ok so here are results:

Changes in file fuzzaldrin.cofee:

Before:

filter: (candidates, query, options = {}) ->
    return [] unless query?.length and candidates?.length
    options = parseOptions(options)
    filter(candidates, query, options)

After:

filter: (candidates, query, options = {}) ->
    return immutable.List() unless query?.length and candidates?.size
    options = parseOptions(options)
    filter(candidates, query, options)

Changes in file filter.coffee are here https://www.diffchecker.com/899ruzir.

So now I'm not sure if we should try to put support for immutable.js into your lib or I should just fork it and call it something like immutable-fuzzaldrin. What do you think @jeancroy?

@jeancroy
Copy link
Owner

I'm looking at the Immutable documentation

If you need to apply a series of mutations locally before returning, Immutable gives you the ability to create a temporary mutable (transient) copy of a collection and apply a batch of mutations in a performant manner by using withMutations
Important!: Only a select few methods can be used in withMutations including set, push and pop.

With this in mind i'd probably adapt the following line to use withMutations so you don't end up creating 300 different immutable list before sorting.

scoredCandidates = scoredCandidates.push({candidate, score})


Pushing on the idea that you want to avoid .toJS for performance reason I think we can do the following:

  • First iterate on the Immutable collection (using forEach)
  • Over each item execute an extract function x=>x.get("key")
  • Collect the score
  • Push to a mutable array of <Immutable Map, Score>
  • Map Pluck / take top N.
  • Reseal the output "array of Immutable map" as "Immuatable collection of immutable map".

Basically let's assume you have 5000 items and at the end want to take the top 10.
I'd much rather pay the price to re-seal the last 10 item then have immutable overhead on the temp array of 5000 items

Moreover this would increase the shared codepath between immutable / classic JS, so the immutable implementation could fit inside two function input transform (extract) and output transform ( array -> immutable collection)

The input /output transform could be re-used to support other data structures, maybe other implementation of immutable, or even complex / computed key object.x.y.z

What do you think ?

@jeancroy
Copy link
Owner

Basically what I'm trying to say is that by nature scoredCandidates is an object that will mutate once per item then .. as soon as it stop mutating... it get discarded with sort pluck and take.

so it's very anti immutable by nature, and not moving this one, I think it's possible to have easier cohabitation between normal, immutable and others.

@petrbrzek
Copy link
Author

@jeancroy Sounds like a better solution. I'm up for that 👍.

@petrbrzek
Copy link
Author

@jeancroy Any idea when this could be implemented?

@jeancroy
Copy link
Owner

Hi @petrbrzek just a head up.

There's now a es6 branch on which I redesigned the filtering.

There'll be support for cancelable async as well as iterating over array, anything that support es6 iterator and anything with .forEach method. They 'key' options also support functions, so one can write x=>x.prop or x => x.get('prop')

I still have to write test & benchmark, but with those changes the library will officially support Immutable.js
Sorry to be about 6 month late on the issue, I work on this as I have free time.

Also, Merry Christmas !

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

2 participants