Skip to content

How to get updated EntityAdapter / EntitySelectors after CRUD operation to get synced operations #452

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

Closed
smajl opened this issue Mar 28, 2020 · 11 comments

Comments

@smajl
Copy link

smajl commented Mar 28, 2020

Consider this pseudo example:

const adapter = createEntityAdapter();
const selectors = adapter.getSelectors(state => state.some);

...
initialState: adapter.getInitialState(),
reducers: {
    remove(state, action) {
       console.log(selectors.selectIds(state).length); // let's say it's 10
       adapter.removeMany(state, action.payload.ids); // ids = [1, 2, 3]
       console.log(selectors.selectIds(state).length); // I would expect 7, but it's still 10 !
    },
...  

I am confused, am I missing something? Do I need to generate new set of selectors after each CRUD operation?

@phryneas
Copy link
Member

Hm, that's an interesting one. As you are within a reducer, that reducer is wrapped in a immer Proxy object.
That proxy object does not change referential identity until the reducer actually finishes, so the memoized selector cannot detect a change and will return the same value over and over again.

Clearly we didn't take into account that someone would use the selectors within a reducer.

@markerikson we could make the selectors use immer's original function before passing it to the reselect selectors, what do you think?

@phryneas
Copy link
Member

@smajl could you report if this fixes your problem? I'm not sure if calling original from within a produce call would already return the modified copy.

+import { original } from 'immer';

const adapter = createEntityAdapter();
const selectors = adapter.getSelectors(state => state.some);

...
initialState: adapter.getInitialState(),
reducers: {
    remove(state, action) {
-       console.log(selectors.selectIds(state).length); // let's say it's 10
+       console.log(selectors.selectIds(original(state)).length); // let's say it's 10
       adapter.removeMany(action.payload.ids); // ids = [1, 2, 3]
-       console.log(selectors.selectIds(state).length); // I would expect 7, but it's still 10 !
+       console.log(selectors.selectIds(original(state)).length); // I would expect 7, but it's still 10 !
    },
...  

@phryneas
Copy link
Member

Hmm. It doesn't seem so, original returns the initial value. I guess we'd have to
override reselects defaultEqualityCheck to do something like

function defaultEqualityCheck(currentVal, previousVal) {
  return currentVal === previousVal && !isDraft(currentVal)
}

But that would lead to new values being generated even if there is no need for it, possibly causing referential inequality of otherwise equal values down the road.

I'll open an issue over at immer on this later. Maybe they have a good idea.

@phryneas
Copy link
Member

@smajl I just noticed what probably was an error creating this example:

You are calling adapter.removeMany(state, action.payload.ids);, are you not? Calling adapter.removeMany(action.payload.ids); like you wrote won't do anything.

@smajl
Copy link
Author

smajl commented Mar 28, 2020

@phryneas Yes, I'm using correct call with state, it was just a "typo" when I was rewriting original into this example.

Anyway, thanks. Btw, my use-case: Tabs list with selected tab index pointer. After removing N tabs from the list, I need to move that index pointer to closest suitable tab based on some logic. All in one reducer.

In the meantime I simplified and refactored my code, so it does not rely on that second selectors call. :) And since I'm now using your nifty EntityAdapter, I may just use tab IDs instead of index and compute index on the fly with selectIds(state).indexOf(id).

@phryneas
Copy link
Member

Good you are finding a way around this for yourself - but you definitely found a tricky potential bug here, so thanks for the report - and we'll continue to try to fix this :)

@markerikson
Copy link
Collaborator

Given the proxy issue, I'm not sure there's much we can do here.

This seems like an unfortunate collision between how Immer works and how Reselect works, and abstractions are leaking.

To be honest, createSelector isn't even actually providing any benefit for selectIds, selectEntities, and selectById, since those are just direct lookups. There's nothing to memoize. selectAll and selectTotal at least are deriving some values.

@smajl
Copy link
Author

smajl commented Mar 30, 2020

@markerikson So the suggested thing to do here is to just directly access your entity adapter {ids, entities} in the state and don't use adapter's selectors for that?

@markerikson
Copy link
Collaborator

Given that the selectors don't provide any benefit here in a reducer, and that you're seeing this issue, yes.

@cqh963852
Copy link

cqh963852 commented Nov 9, 2020

remove(state, action) {
       console.log(selectors.selectIds(state).length);
       const nextState = adapter.removeMany(state, action.payload.ids);
       console.log(selectors.selectIds(nextState).length);
}

Here is my solution.

@cqh963852
Copy link

I meet the same problem

https://codesandbox.io/s/intelligent-proskuriakova-198ge

image

The data in nextState has been changed,but the selectById function seems cached the prevState.

Does pure function much better than a cached function?

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

4 participants