Extract sort function from Collection types

[bdurrer]

Follow-up of #1888:

Map includes OrderedMap because of the sort function. Same with Set and OrderedSet. This leads to a circular dependency, which makes tree-shaking ineffective

[jdeniau]

I will add this here too as I did merge the PR reverting three shaking:

For the record the sideEffects I set was wrongly set to "./src/CollectionImpl.js" instead of "./dist/es/CollectionImpl.js"

Once you include CollectionImpl, then all .toXXX methods are implemented and the bundle size goes back to 17ko instead of 18ko (full bundle), so nearly no gain.
Even with the CollectionImpl as side effect, I had on issue on this code

where Collection is undefined.

[iamrenejr]

Hello! It's a shame this blocks tree shaking. If I understand the discussion so far including from the other threads, here are the issues:

• Map includes OrderedMap via the sort method
• Set includes OrderedSet via the sort method
• Collection imports every .toX method

What sort of final solution are we looking at? For example, given it will be a breaking change, is it acceptable to remove these methods from the base class and provide a utility function that will do the conversion? Such that:

• Map.sort is factored out into mapToOrderedMap (which will import both Map and OrderedMap)
• Set.sort is factored out into setToOrderedSet (which will import both Set and OrderedSet)
• Collection.toX is factored out into collectionToX (which will import both Collection and X)

Some variations of this API that make it more standard could also be implemented, such as doing something like:

type ToOrdered = {
  <T>(obj: Map<keyof T, T[keyof T]> factory: OrderedMapFactory): OrderedMap<keyof T, T[keyof T]>;
  <T>(obj: Set<T>, factory: OrderedSetFactory): OrderedSet<T>;
};

const toOrdered: ToOrdered = (obj, factory) => {
  // Sorting logic and transformation to target constructor goes here
  // Ideally, the sorting logic should be the same regardless of the given object's type due to standard APIs being the same
  const result = /* sorted result */;
  return factory(result);
}

I'm not familiar with the underlying code behind immutable.js, so please take this with a grain of salt! But is a solution like this what we're looking for, or are we thinking of another approach?

[jdeniau]

About breaking the API by removing toXxx, I am not a fan of removing those methods, as we will loose some developer experience here, and be inconsistent with the immutable logic here : those helpers are really great when your want to chain calls :

Map(xxx) 
 .sort()
 .getValues()
 .toList()
 .slice(1);

Is dynamic import a doable think in library that might help here?

[iamrenejr]

I see! Well, clearly I wasn't thinking about it from a library maintenance perspective, and instead was just seeing it from my own perspective as a user of the library! Thanks for clarifying the downsides of my suggestion!

I wasn't going to make use of the chaining syntax myself. I was piping and composing utilities together, so for me, your example would have looked something like this:

const toSort = (obj) => obj.sort();
const getValues = (obj) => obj.getValues();
const toList = (obj) => obj.toList();
const slice = (num) => (obj) => obj.slice(num);

const pipeline = pipe(
  toSort,
  getValues,
  toList,
  slice(1),
);

pipeline(Map(xxx));

This isn't really how I'm using immutable.js though! I'm mostly just making use of its .get and .set method to make lenses with Ramda. So like:

const immutableJSLens = (prop) => lens(
  (store) => store.get(prop),
  (value, store) => store.set(prop, value),
);

const ALens = immutableJSLens('a');
const BLens = immutableJSLens('b');
const ABLens = compose(ALens, BLens);

const immutableState = fromJS({ a: { b: 1 } });

set(ABLens, 2, immutableState); // { a: { b: 2 } }

I'm realizing now that the full weight of immutable.js may be too heavy for the simple application I'm trying to use it for. I don't think dynamic import will work since I want to initialize my state as an immutable object right away, and that would have to happen before the UI loads.

[bdurrer]

I'm realizing now that the full weight of immutable.js may be too heavy for the simple application I'm trying to use it for.

I agree. I would say ImmutableJS is great if you use it to deal with Collections (aka complex manipulations, filter, sort, chaining), but just to make immutable objects, it's overkill. There are simpler "freeze" type libraries for that.

[iamrenejr]

Thanks for the clarification!