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

[DRAFT] Experiment: Add "indices" to entity adapter #948

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

markerikson
Copy link
Collaborator

Inspired by #799 , I'm trying to add some "indices" to createEntityAdapter. The idea is that you could define secondary sorting options beyond the standard sorting for state.ids, and the entity adapter will automatically keep those updated.

I initially was looking at redux-indexers, but decided that it wasn't quite a good fit.

I'm now playing with the idea of having the sorting get done inside of the merge() function in sorted_state_adapter.

Was having trouble getting the fields of the indices option to carry through, but @msutkowski just got that working in #946 .

Still got a lot of experimenting left to do.

@netlify
Copy link

netlify bot commented Mar 28, 2021

Deploy preview for redux-starter-kit-docs ready!

Built with commit c6be881

https://deploy-preview-948--redux-starter-kit-docs.netlify.app

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 33ea7a2:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration

@github-actions
Copy link

Size Change: +521 B (0%)

Total Size: 70.3 kB

Filename Size Change
dist/redux-toolkit.cjs.development.js 14 kB +112 B (0%)
dist/redux-toolkit.cjs.production.min.js 5.4 kB +99 B (1%)
dist/redux-toolkit.esm.js 13.9 kB +114 B (0%)
dist/redux-toolkit.umd.js 25.8 kB +112 B (0%)
dist/redux-toolkit.umd.min.js 11.1 kB +84 B (0%)
ℹ️ View Unchanged
Filename Size Change
dist/index.js 149 B 0 B

compressed-size-action

@markerikson
Copy link
Collaborator Author

markerikson commented Mar 28, 2021

Got something kinda working here. 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.

Things that are missing:

  • Generating additional select$IndexName selectors automatically
  • Seeing if we can add a filtering option as well, ie:
indices: {
  usersOver40ByAge: {
    sortComparer: (a, b) => a.age - b.age,
    filter: user => user.age >= 40
  }
}

@sabarnix
Copy link

indices: {
  usersOver40ByAge: {
    sortComparer: (a, b) => a.age - b.age,
    filter: user => user.age >= 40
  }
}

I have a concerns with this. How to handle cases where filtering is dependent on some other key in state or url param. Let's say 40 (the filtering age) is user input and sort order is stored in query params. Another problem would be to determine when the dependent param has changed.

@markerikson
Copy link
Collaborator Author

@sabarnix I think at that point you'd need to do the remaining filtering outside the adapter.

Assuming this is being used as a slice reducer, you only have access to the current slice state + action anyway, and at this point in the code, you don't even have access to the rest of the slice state. So, if we decide to support any filtering at all, I think it's going to be limited to just what you can do based on a single item itself.

@dutzi
Copy link
Contributor

dutzi commented Sep 2, 2022

Hey @markerikson, we have a use case for this: entities are "Segments" of shape { startTime: number; endTime: number } that need to be stored sorted by both startTime and endTime.

Currently we use a selector for getting them sorted by endTime, and let RTK manage startTime sorting, but that means we re-sort the entire array on updates (instead of pushing the updated items into a sorted array, efficiently)

I've been reading through this PR, to see if we could help, but didn't find any actionable items.

Can you please give guidance, so we could help make this happen?

Correction: I now see that the typings need some sorting out, we'll try to help with that, if that's ok.

@markerikson
Copy link
Collaborator Author

@dutzi tbh this is entirely backburnered right now. We're focusing on finalizing RTK 1.9, and after that we'll be working on RTK 2.0 that drops back compat for IE11 and such.

The biggest issue here is that there isn't a good definition or requirements for what the desired behavior should be, and that also means that there isn't a valid implementation either. This was a bit of an experiment, but definitely not ready for actual use.

If someone wants to think this topic through further and propose some API designs and behaviors, we can talk about it, and I'm definitely open to PRs. But otherwise I don't see this happening any time soon.

@dutzi
Copy link
Contributor

dutzi commented Sep 2, 2022

Got it. Thanks for the quick response!

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

Successfully merging this pull request may close these issues.

4 participants