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

More advanced access patterns for createEntityAdapter? #799

Open
lukeramsden opened this issue Nov 10, 2020 · 28 comments
Open

More advanced access patterns for createEntityAdapter? #799

lukeramsden opened this issue Nov 10, 2020 · 28 comments
Labels
enhancement New feature or request

Comments

@lukeramsden
Copy link

lukeramsden commented Nov 10, 2020

Thank you for your work on redux-toolkit, it really is an amazing library.

I'm using createEntityAdapter to keep my data normalised and it's working very nicely, however, my use-case means not only do I need to access by ID, but also other fields. I'm currently having to maintain my own "indices" based on various fields of my entities.

My entity looks like this:

interface MyEntity {
	// primary key, handled nicely by createEntityAdapter
	id: string;
	// also handled by createEntityAdapter via sortComparer
	createdAt: string;

	//...content stuff, not relevant

	// the juicy bit
	parent: string | null;
}

I need to be able to quickly access a list of entities in one of 2 ways:

  1. Where parent is null, for the index of top level entities with no parent
  2. Where parent is equal to x, for the children of a certain entity

I'm currently maintaining my own two indices for this.

interface EntitiesState extends AsyncFetchReduxState {
  idsWhereParentIsNulll: EntityId[];
  idsByParent: Record<EntityId, EntityId[]>;
}

I then have to make sure these are both kept in sync wherever I'm updating the adapter state - not to mention keeping them sorted, which I haven't got round to yet.

// root entities, pretty simple
builder.addCase(fetchEntitiesWithNoParent.fulfilled, (state, action) => {
  state.status = 'succeeded';
  entitiesAdapter.upsertMany(state, action.payload);

  for (const entity of action.payload) {
    if (!state.idsWhereParentIsNulll.includes(entity.id)) {
      state.idsWhereParentIsNulll.push(entity.id);
    }
  }
});

// child entities, a little more complex
builder.addCase(fetchStoriesByParent.fulfilled, (state, action) => {
  entitiesAdapter.upsertMany(state, action.payload);

  const parent = action.meta.arg;
  const parentArrayExists = parent in state.idsByParent;
  if (!parentArrayExists) {
    state.idsByParent[parent] = [];
  }
  for (const entity of action.payload) {
    // push entity ID in to array
    const entityAlreadyInHashmap = state.idsByParent[parent].includes(
      entity.id,
    );

    if (!entityAlreadyInHashmap) {
      state.idsByParent[parent].push(entity.id);
    }
  }
});

Add in keeping the list sorted by createdAt, and many more call-sites updating these entities due to pagination, refreshing, etc. its quite easy to introduce de-sync bugs. This could be handled as computed state with a selector, which is what I started with, however due to it being, in my case, a mobile app, sessions are long-lived and a user may fetch a lot of content, even if they're only accessing a little, due to the tree-like nature of my data.

I'll probably introduce helper functions that take in the state, and maybe the adapter, and perform these actions as sort of wrappers for the CRUD functions - however, is it plausible that redux-toolkit could handle these sorts of use-cases natively? A "filtered ID array" index and a "ID array grouped by field" would be enough for my use-case, however there may be other use-cases it could open the door for. Some interesting inspiration for this is https://github.com/corps/redux-indexers with it's multi-level indexes, which is a nice little utility, and may work very well for this although I haven't tried it yet.

And I will also say in advance that I'm not adamant this is the best solution to my own use-case, or that this should definitely be handled by redux-toolkit, but I am interested on what people who have a lot more experience with React and Redux than me would have to say about this.


EDIT: another use-case could be having multiple indices for different sort options

@markerikson
Copy link
Collaborator

I can definitely see the potential use cases for maintaining additional indices. That said, this is the first time anyone's asked for something like that. So, my off-the-cuff response is that I'd like to see some API proposals and proof of concepts first, and then discuss specifics and whether it's something that's actually worth including in RTK directly.

Frankly, we've already gone well past a couple limits that I had originally listed for RTK :) I wrote up a "My Vision for RTK" statement almost two years ago, and at that time I said:

Some specific use cases this library will not address:

-Full-blown querying of APIs and normalizing entity data. There's too much variety in how people handle data, including where it comes from, how they normalize it, how the loading status is tracked, etc. I know the NgRx folks have @ngrx/entity and ngrx-data that manage fetching, normalization, selectors, and everything else. I understand the desire to abstract away everything regarding handling "business entities", but that requires a ton more time and effort that I can put into this, and I don't want that much complexity involved. The closest I would even remotely consider is maybe including normalizr out of the box, and even that is more than I'd really prefer to do here. (Insert "blah blah GraphQL" comment here.)

Well... we've already got createEntityAdapter, and @phryneas and company are currently working on a full-blown query / caching abstraction over at https://github.com/rtk-incubator/simple-query . Definitely not where I thought we were going to end up :)

@lukeramsden
Copy link
Author

Thank you for the great response. simple-query looks very promising, I look forward to seeing how that pans out.

As for an API proposal, here's a first draft of what I'd imagine, with a few possible options for each part:

// initialisation
const entitiesAdapter = createEntityAdapter<TEntity>({
  selectId: (entity) => entity.id,
  sortComparer: (a, b) => b.createdAt.localeCompare(a.createdAt),
  indexes: {
    whereParentIsNull: {
      byFilter: (entity) => entity.parent === null,
    },
    byParent: {
      // as function, easy to type
      byField: (entity) => entity.parent,
      // easier to use, not sure if possible to type via inference
      // both are probably necessary as being able to derive the field may be useful?
      byField: 'parent',
    }
  },
  // or
  indexes: [
    {
      name: 'whereParentIsNull',
      byFilter: (entity) => entity.parent === null,
    },
    {
      name: 'byParent',
      
      byField: (entity) => entity.parent,
      // or
      byField: 'parent',
    }
  ]
});

// regular actions like addOne handle keeping the indexes up to date
// maybe a `skipIndex` param may be useful but I wouldn't know why

// Access
// Export the customised selectors for this adapter using `getSelectors`
export const {
  selectAll: selectAllEntities,
  selectById: selectEntityById,
  selectIds: selectEntityIds,
  // Pass in a selector that returns the entities slice of state
} = entitiesAdapter.getSelectors<RootState>((state) => state.entities);

// What I currently have
export const selectEntityIdsByParent = (parent: NonNullable<TEntity['parent']>) =>
  createSelector(
    (state: RootState) => state.entities,
    (state) => state.idsByParent[parent] || [],
  );
export const selectEntityIdsWithNoParent = createSelector(
  (state: RootState) => state.entities,
  (state) => state.idsWhereParentIsNulll,
);
// What we could have
export const selectEntityIdsWithNoParent = entitiesAdapter.getIndexSelector<RootState>('whereParentIsNull')
export const selectEntityIdsByParent = entitiesAdapter.getIndexSelector<RootState>('byParent')

// Or we could export the selectors as the names of the indexes
// So index names are `selectWhereParentIsNull` and `selectByParent`, and then we have:
export const {
  selectAll: selectAllEntities,
  selectById: selectEntityById,
  selectIds: selectEntityIds,
  selectWhereParentIsNull: selectEntityIdsWhereParentIsNull,
  selectByParent: selectEntityIdsByParent
  // Pass in a selector that returns the entities slice of state
} = entitiesAdapter.getSelectors<RootState>((state) => state.entities);

I'm not sure how easy this would be to type, I've not delved in to any of the more advanced generic and inference stuff in TS yet.

As for a proof of concept, what form would that take? A rough implementation within this codebase? Tests with no implementation TDD style?

@markerikson
Copy link
Collaborator

Mm... first question, I think, is about the actual state design and behavior.

What form should these additional indices take? Objects? Arrays? Dealing with just the IDs, or the actual item values? Are they supposed to be sorted in some way? Are these supposed to be kept updated after every operation?

Beyond that, it's always easiest to talk about actual semi-working code :) agreed that the TS typing is probably going to be the most difficult part atm, but if you can try tackling it and fake out some of the types for now just to get the code actually updating whatever state you're envisioning, that would give us a starting point for further discussion.

@lukeramsden
Copy link
Author

What form should these additional indices take? Objects? Arrays?

Both. Could be an array of IDs filtered, or maybe even sorted too, a different way, or an object with keys derived from the data equalling arrays of IDs filtered and sorted.

Dealing with just the IDs, or the actual item values?

In state, just IDs. Along with the selectors that return arrays of IDs, we could also have ones that return the full entities if a user so desires.

Are they supposed to be sorted in some way?

I think the option should be there, the same way it is for the regular index of entities the adapter creates.

Are these supposed to be kept updated after every operation?

Yes, they would be kept updated automatically after every operation by the adapter itself. Since it's all just based on the data the user is already putting in through the regular entity adapter, we could, and I started with this, create selectors that use map, filter, and sort to get this same information, in the same way we could get the entityIds field from the normal adapter to get an array of all the IDs, but that just doesn't quite feel right from a performance perspective.


Beyond that, it's always easiest to talk about actual semi-working code

Fair enough. I'll see if I can look at getting to grips with the codebase and getting some sort of implementation going, although I don't know when that'll be as it'll probably have to be outside of work hours for now. I'll update this issue as and when.

@grgmgd
Copy link

grgmgd commented Dec 14, 2020

I like the mentioned approach but had a different one in mind to abstract the filtering a bit. if you allow me.
Can we reduce this into a simpler version by allowing multiple id arrays inside the state and adding support for dynamic array editing inside createEntityAdapter, example state:

{
   "ids": [],
   "withParent2": [],
   "withParentNull": [],
   "entities": {}
}

If we define an extra parameter for the CRUD operations functions as follows:

updateMany(state: S, updates: PayloadAction<Update<T>[]>, key: string)

Key values could be withParent2, withParentNull then update the corresponding array. If no key was provided the default array ids is updated as it is right now.
This way it's left up to the reducer to handle the filtering but with the added benefit of the entity adapter's CRUD operations.

If you like this approach I can start working on a codebase right now and submit a pull request.
If it's not a nice approach, @lukeramsden if you need any assistance with your approach I want to help.

@markerikson
Copy link
Collaborator

Adding extra args here won't work because those functions need to work as standalone reducers.

@grgmgd
Copy link

grgmgd commented Dec 14, 2020

@markerikson forgot about this part 😅 I'll try and come up with a solution and update when done. Thank you for the amazing package!

@markerikson
Copy link
Collaborator

FWIW, I'm still open to this and any other ideas for improving createEntityAdapter - I just haven't seen any further discussion going on here.

@sabarnix
Copy link

sabarnix commented Mar 25, 2021

For my use case, on search I get the new list from server. I think simple approach to tackle most use cases would be

createAdapter

const catsAdapter = createEntityAdapter({
  selectId: (cat) => cat.id,
})

initial state

const initialState = catsAdapter.getInitialState({
  defaultIds: [],
  searchResultIds: [],
  searchText: '',
  isLoading: false,
})

reducer

const catSlice = createSlice({
  fetchedCats(state, { payload }) {
    const { ids, entities } = catAdapter.setAll(state, payload);
    state.defaultIds = ids;
  },
  searchedCats(state, { payload }) {
    const { ids, entities} = catAdapter.addMany(state, payload);
    state.searchResultIds = ids;
  }
})

search result reselect selectors can be created from entities selector and state.searchResultIds.
only addition for this required is to make the adapter methods return ids and entities.

@markerikson
Copy link
Collaborator

Sorry, I'm not sure what API or behavior changes you're trying to show in that example. Can you clarify what you're describing?

@sabarnix
Copy link

As you can see in my example I am trying to maintain 2 sets of ids searchResultIds and defaultIds but both share the same entity map.

@markerikson
Copy link
Collaborator

Okay, but I don't understand what you're proposing in terms of a second "ID index" here. It would help if you could describe what you're expecting to have happen, preferably with some examples for what those return values would contain.

@sabarnix
Copy link

What I am trying to say is if all the update methods of createEntityAdapter (updateOne, updateMany, ...) can return the id list and the entity object (as an object or array) then we can derive the desired id list from them and maintain it in a separate key in the store. Selectors for them can also be created easily by composing custom selector with entityAdapter selector.

@markerikson
Copy link
Collaborator

Not really sure how that helps.

For one thing, the API adapter CRUD methods already have defined return values, so we can't change those.

Second, the CRUD methods either "mutate" the existing state (if it's an Immer draft) or return a new state result (if they're being used standalone).

Third, the updated state either way is going to have the complete list of IDs, not just some subset.

So, I'm still not clear what specific results you're expecting to see here that would make additional indices easier to manage, and we can't change the API signatures because that would be a breaking change.

If you could give me a specific example (fake code, but actually showing example values in the arrays and results and stuff), it might help me understand what you're asking for here.

@sabarnix
Copy link

For @lukeramsden's problem I think it's not a good idea to maintained filtered id list in state. Rather that can be done using an reselect selector.

const makeSelectIds = (state) => entityAdapter.selectIds(state.stateKey);
const makeSelectEntities = (state) => entityAdapter.selectEntity(state.stateKey);

const makeSelectWithNoParent = () =>
  createSelector(
    makeSelectIds,
    makeSelectEntities,
    (ids, entities) => ids.filter((id) => !entities[id].parent )),
  );

@markerikson
Copy link
Collaborator

There are times when maintaining an additional filtered list of IDs might be useful. See https://medium.com/@dcousineau/advanced-redux-entity-normalization-f5f1fe2aefc5 .

@sabarnix
Copy link

sabarnix commented Mar 25, 2021

Not really sure how that helps.

For one thing, the API adapter CRUD methods already have defined return values, so we can't change those.

Second, the CRUD methods either "mutate" the existing state (if it's an Immer draft) or return a new state result (if they're being used standalone).

Third, the updated state either way is going to have the complete list of IDs, not just some subset.

So, I'm still not clear what specific results you're expecting to see here that would make additional indices easier to manage, and we can't change the API signatures because that would be a breaking change.

If you could give me a specific example (fake code, but actually showing example values in the arrays and results and stuff), it might help me understand what you're asking for here.

I use immer in my project so missed the case where the update method returns the new state after update. Adding one method to get the normalised data will help.

eg.

const { ids, entities} = catAdapter.normalise(payload);

updated reducer

const catSlice = createSlice({
  fetchedCats(state, { payload }) {
    catAdapter.setAll(state, payload);
    const { ids, entities } = catAdapter.normalise(payload);
    state.defaultIds = ids;
  },
  searchedCats(state, { payload }) {
    const { ids, entities} = catAdapter.addMany(state, payload);
    const { ids, entities } = catAdapter.normalise(payload);
    state.searchResultIds = ids;
  }
})

@markerikson
Copy link
Collaborator

We're not looking to implement a complete normalization solution of arbitrary incoming data. For that, there's https://github.com/paularmstrong/normalizr , and we have a usage guide section that suggests how to use it with RTK: https://redux-toolkit.js.org/usage/usage-guide#normalizing-with-normalizr

None of this seems to actually relate to keeping some additional sub-index via the entity adapter that I can see. Please re-read the original comment in this thread to see what the problem statement is that we're discussing.

@sabarnix
Copy link

Adding extra args here won't work because those functions need to work as standalone reducers.

what if we allow a partial curried function

.key(key: string).updateMany(state: S, updates: PayloadAction<Update<T>[]>)

without the .key updateMany can continue to work as it is now

@markerikson
Copy link
Collaborator

markerikson commented Mar 25, 2021

Hmm. Okay, that's at least something concrete that I can look at, but I'm not following the intended behavior exactly. In particular, this "key" parameter that you're passing in seems like a very different concept than what's being discussed in the original comment.

createEntityAdapter specifically maintains a single ID array, which may be sorted. A typical example would be "sort items by name", in which case the IDs array is kept updated in a sorted order based on the item.name fields as items are added, updated, and deleted.

The question at hand is if it's possible to define additional indices, such as (fake code):

const booksAdapter = createEntityAdapter<Book>({
  // Assume IDs are stored in a field other than `book.id`
  selectId: (book) => book.bookId,
  // Keep the "all IDs" array sorted based on book titles
  sortComparer: (a, b) => a.title.localeCompare(b.title),
  // FAKE EXAMPLE HERE
  indices: {
    byAuthor: (a, b) => a.author.localeCompare(b.author),
    byYear: (a, b) => a.year - b.year
  }
})

Those indices would then also be automatically updated as items are added, updated, and removed.

What you're showing is something very different. It looks like you're trying to associate certain IDs with specific keys, but only doing so in specific operations? I'm really not sure what the semantics are that you're proposing here.

I think the biggest difference is that the original discussion is looking at trying to auto-maintain these indices based on attributes that are already in the entity objects themselves, whereas your suggestion seems to be about attaching additional metadata instead (or something like that).

As a side note: @lukeramsden , I missed that you'd liked to redux-indexers in your comment. I hadn't seen that before, and I think it does look very intriguing.

@sabarnix
Copy link

We can keep the structure exactly like https://medium.com/@dcousineau/advanced-redux-entity-normalization-f5f1fe2aefc5 . and not change the createEntityAdapter constructor at all.

current structure

{
  ids: [],
  entities: {}
}

new structure

{
  keys: [],
  ids: [],
  entities: {}
}

so now on say setAll

.key('gryffindor').updateMany(state, [{ id: 1, name: 'harry potter' }, { id: 2, name: 'Dumbledore' } ])

updated state

{
  keys: ['gryffindor'],
  ids: [],
  gryffindor: [1, 2]
  entities: {
   1: { id: 1, name: 'harry potter' },
   2: { id: 2, name: 'Dumbledore' }
  }
}

@markerikson
Copy link
Collaborator

I still don't understand what this "key" thing is you're proposing. I don't know what the intended semantics are, and I don't know what purpose it's supposed to serve.

I can't even consider adding something if I don't know what it's supposed to do and how it's supposed to work. At a minimum, you'd need to explain exactly what that does, what problem it solves, and how it's supposed to behave.

@sabarnix
Copy link

sabarnix commented Mar 25, 2021

I think memoized selectors are the best way to solve the original problem of accessing by different keys. Which is basically multiple views filtered from state data.

Regardless let me try to solve the original problem by maintaining idsWhereParentIsNulll in a different key

create entity

const entitiesAdapter = createEntityAdapter<TEntity>({
  selectId: (entity) => entity.id,
  sortComparer: (a, b) => b.createdAt.localeCompare(a.createdAt),
});

AddEntities

entitiesAdapter.key('idsWhereParentIsNulll').setAll(store, payload.filter((entity) => entity.parent ===  null))

Store structure

{
  keys: ['idsWhereParentIsNulll'],
  idsWhereParentIsNulll: EntityId[],
  entities: Record<EntityId, Entity>;
}

Basically, I don't think we should store the configuration of how to populate the derived id lists. Rather we should allow specifying the key which will keep the ID list. The reason being the filtering logic of the sub id lists may be dependent on some other key in the state or some prop which can't be passed with the configuration. This way entityAdapter will not have to take the responsibility of updating the sub id lists on every update.

The problem which I am trying to solve is to maintain the list of ids in multiple keys for multiple views. Let's say for server side searching of an entity list. In the original key ids we can maintain the default list of ids and then on search we can extract the ids from search api response and keep it in searchResultIds key. This way we can maintain bot the state in store and once user clears the search query we can quickly render the default list from ids key without having to make another api call.

@markerikson
Copy link
Collaborator

I don't like this idea of doing adapter.key().setAll(). That seems like a completely different API approach than what we have now, and I don't think it's solving anything useful.

@sabarnix
Copy link

I was just trying to think of a way to implement this using createEntityAdapter. I will get back if I can think of a better solution.
Thank you for your time and this awesome package!

@markerikson
Copy link
Collaborator

My inclination at this point would be to see if we can leverage that https://github.com/corps/redux-indexers package linked in the original comment. If we did, we'd probably port the logic over to RTK instead of adding it as a dependency.

And sure :)

@markerikson
Copy link
Collaborator

@lukeramsden : so I've got something sorta-kinda working over in #948 . Would appreciate it if you could try out installing the build from CodeSandbox CI in that PR, and try this out.

Types still aren't right yet - it's showing fields in entityState.indices that don't actually exist. But the logic seems like it's mostly there.

@alexkrolick
Copy link

alexkrolick commented Mar 28, 2021

Re: #948 as of c6be881

Example:

createEntityAdapter({
      sortComparer: (a, b) => { } // omit,
      indices: {
        byId: (a, b) => http://a.id.localeCompare(http://b.id),
        byTitleLength: (a, b) => a.title.length - b.title.length
      }
    })

This results in a state of:

{
  ids: [],
  entities: {},
  indices: {
    byId: [],
    byTitleLength: []
  }
}

and those additional indices are also ID arrays, kept sorted as you make updates to the items


I am not sure this is an "index" quite yet. A database index would provide an alternate lookup key and not just an efficient alternate scan method (sort). For example, byTitleLength(length: Int): [PrimaryKey] would actually allow querying for books by their (indexed) length (and in theory, by ranges as well). The data structure representation of this for the database would be some type of tree that provides efficient traversal and lookup, which isn't really exposed because the query language takes care of it. It's a bit difficult to do that in plain JS data structures. I'll update this comment if I think of anything.

// API (methods, not the data structures backing them)

const shortTitleIds = books.byTitleLength(5, 10); // this queries the index
// ['b_123', 'b_456', 'b_001']
books.entities['b_123']
// { title: 'FooBar', id: 'b_123' }

const allBookIdsSortedByTitleLength = books.byTitleLength(); // this scans everything and uses the index sort
// ['b_123', 'b_456', 'b_001', 'b_098', 'b_1234']
books.entities['b_1234']
// { title: 'really long title', id: 'b_1234' }

// Selectors can use the index to get IDs instead of just the primary ID key or list of IDs
// However, without a uniqueness constraint on the index can only return arrays of items
const booksWithShortTitles = selectEntitiesById(books.byTitleLength(6));
// [ { title: 'FooBar', id: 'b_123' } ]

// Passing index result to ID selector is a bit indirect so maybe a default selector-like method could be added for each index
const booksWithShortTitles = books.selectByTitleLength(6);
// [ { title: 'FooBar', id: 'b_123' } ]

EDIT:

I think a pretty good data structure for this would be a sorted array (balanced tree) that takes the comparison function and stores an object reference or a structure like (secondaryKeyValue, primaryKeyValue)

Rough example implementing a getRange() value on top of this SortedArray implementation: https://replit.com/@alexkrolick/HonoredEasySorting#index.js

@markerikson markerikson added the enhancement New feature or request label Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants