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

suggestion: add overload for "solo selector" without input selectors #644

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phryneas
Copy link
Member

This is just something I wanted to explore.

Of course, people can always just call defaultSelector, but that's an additional function people have to be aware of, and it makes code bases less uniform. Right now, people oftentimes use identity functions, either as input selectors, or worse, even as result functions.

This would just allow a single function without input selectors to be memoized directly.

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 b099da9:

Sandbox Source
JavaScript Configuration
Vanilla Typescript Configuration

@aryaemami59
Copy link
Contributor

Very interseting idea, do you think it would be simpler to just have a new API called something like createSoloSelector or createSimpleSelector instead of adding another function signature to CreateSelectorFunction?

@phryneas
Copy link
Member Author

That would defeat the purpose.

People write bad code like

const selector = createSelector(
  state => state.slice,
  slice => slice
  )

or

const selector = createSelector(
  state => state,
  state => state.slice
)

because they don't know any other api.

They could already write

const selector = defaultMemoize(state => state.slice)

but they don't do it, because they don't know about defaultMemoize.

If we now offered

const selector = createSoloSelector(state => state.slice)

it would face the same problem - people don't know about it and won't discover it. They will resort to the bad alternatives above instead.

That's why I suggest

const selector = createSelector(state => state.slice)

here to "just work" - it solves the problem at the root without introducing mental overhead.

@aryaemami59
Copy link
Contributor

That's a good point, do you think #645, will help mitigate some of the problem?

@aryaemami59
Copy link
Contributor

@phryneas Do you want me take a look at this to see if I can get the types to work?

@phryneas
Copy link
Member Author

@aryaemami59 I think #645 would help people spot the error, but currently it would still send people to use another function, which still adds the mental overload, so I think these two would complement each other.

As for fiddling with the types here: please go ahead, I don't currently have the time to take a deep dive here, and you're much more familiar with the code base. I assume that changing the signature order might already mitigate the warnings, but I could be wrong :)

@aryaemami59
Copy link
Contributor

@aryaemami59 I think #645 would help people spot the error, but currently it would still send people to use another function, which still adds the mental overload, so I think these two would complement each other.

As for fiddling with the types here: please go ahead, I don't currently have the time to take a deep dive here, and you're much more familiar with the code base. I assume that changing the signature order might already mitigate the warnings, but I could be wrong :)

I do not possess your skills, but I will certainly try my best :)

@markerikson
Copy link
Contributor

markerikson commented Nov 26, 2023

FWIW I'd like to defer consideration of this one until after RTK 2.0 / Reselect 5.0 is out. It isn't critical, it's something we could do in a minor, but I'm also still not convinced it's something we should do atm.

My top priority atm is doing some more scenarios to test how weakMapMemoize behaves (recreating the memory leak possibility, checking if we need resultEqualityCheck, checking how it works as a shared selector for list item components, etc)

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.

3 participants