-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Extract sort function from Collection types #1961
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
Comments
I will add this here too as I did merge the PR reverting three shaking: For the record the Once you include where |
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:
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:
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? |
About breaking the API by removing Map(xxx)
.sort()
.getValues()
.toList()
.slice(1); Is dynamic import a doable think in library that might help here? |
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 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. |
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. |
Thanks for the clarification! |
I think that zod did have the same problem as we have, and that they solved it in a way that seems really interesting here : they created two packages See the release blog post But if you are OK with loosing the chaining option, you can use zod/mini and wrap in We might do the same thing with immutable here : - List(...).toSet()
+ Set(List(...)) It may be more complicated for functions like groupBy(List(...), grouperFunction) |
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 ineffectiveThe text was updated successfully, but these errors were encountered: