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

change defaultMemoize's isEqual to compare results instead of arguments and rename to comparerFn #1157

Closed
alex-okrushko opened this issue Jun 26, 2018 · 3 comments

Comments

@alex-okrushko
Copy link
Member

I'll start with this: I'm NOT suggesting to remove argument comparison, arguments should be compared anyway and comparing them based on === should be sufficient to start the result evaluation.

However the provided isEqual function should be comparing the results (lastResult + new result), when the arguments comparison doesn't pass.

Reasoning:

  1. Comparing arguments is almost useless, especially if there are two or more arguments coming in.
    Consider the following:
enum OrderType {
  EXEC_LEVEL,
  INTERNAL,
  PUBLIC,
}

interface Order {
  type: OrderType, // <-- this is the enum above
  id: string,
  totalCost: string,
}

// Provides Order[] with all existing orders.
const getAllOrders = createSelector(someSelectorWithOrders, (state) => state.orders);

// Provides currently selected OrderType (the one from the enum).
const getSelectedType = createSelector(someSelectorWithTypes, (state) => state.currentType);

// Assume we have the following comparer, which would just LOGs what that it gets to compare.
function isMyCustomComparer(a: any, b: any) {
  console.log('a:', a, 'b:', b);
  return a === b; // this is what is currently isEqualCheck is doing anyway.
}

// This is how we use the default Memoizer, but are providing custom `isEquals`,
// which is a second argument in the defaultMemoizer function.
const customMemoizer = (aFn) => defaultMemoize(aFn, isMyCustomComparer);

// The actual select of interest. Returns Orders filtered by Current type.
const getOrdersBySelectedType = 
    createSelectorFactory(customMemoizer)(getAllOrders, getSelectedType, 
       (orders: Order[], currentOrderType: OrderType) => 
            orders.filter(order => order.type === currentOrderType));

Let's say we have the following global state

{
   ...
   orders: [{type: OrderType.INTERNAL, id: '1', totalCost: '10.00'},
            {type: OrderType.PUBLIC, id: '2', totalCost: '100.00'}],
   ...
   currentType: OrderType.INTERNAL,
}

Now let's call it twice and see what console.log('a:', a, 'b:', b); inside isMyCustomComparer will produce.

getOrdersBySelectedType({...originalState});
getOrdersBySelectedType({...originalState});

The output we get is:
screen shot 2018-06-26 at 6 15 14 pm

That's because both getAllOrders and getSelectedType go through that isMyCustomComparer

So basically when there are 2 or more "arguments" there could be nothing common for them to compare against.

  1. Comparing the final result will be a lot more beneficial.

E.g. say another order get added, of OrderType.EXEC_LEVEL. That would cause getAllOrders to recompute, and, as a result, getOrdersBySelectedType to recompute and produce another array with the same values (because this added order is does not match my filtered type).
Instead of that, I would like to provide the function like

function isMyCustomComparer(a: any[], b: any[]) {
  return a.length === b.length && a.every(fromA => b.includes(fromA));
}

So that if resulting array has the exact same values then give me the previous result, so that emits are not triggered.

Describe any alternatives/workarounds you're currently using

There are 2 workaround currently:

  1. Wrap the selector with another selector
// Notice, that createSelector is used here, not createSelectorFactory
const getOrdersBySelectedTypeIntermediate = createSelector(getAllOrders, getSelectedType, 
       (orders: Order[], currentOrderType: OrderType) => 
            orders.filter(order => order.type === currentOrderType));

const getOrdersBySelectedType = createSelectorFactory(customMemoizer)(getOrdersBySelectedTypeIntermediate, filteredOrders => filteredOrders);
  1. Provide custom memoization. I won't be showing this, but basically it would look similar to defaultMemoizer but more error-prone and has to use the magically appearing arguments within the function as well.

If accepted, I would be willing to submit a PR for this feature

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@brandonroberts @MikeRyanDev I would like to change that if you don't mind. It will be a breaking change if anyone is currently providing isEqual to defaultMemoize, but I really doubt that anyone does.

@alex-okrushko
Copy link
Member Author

here is the stackblitz: https://stackblitz.com/edit/typescript-uxmzho

@alex-okrushko
Copy link
Member Author

@brandonroberts Thoughts?

@timdeschryver
Copy link
Member

Closed via #1175

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

No branches or pull requests

2 participants