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

Add selectors #518

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add selectors #518

wants to merge 4 commits into from

Conversation

ryoid
Copy link

@ryoid ryoid commented Sep 24, 2023

Using createSelector is good for a single values. I found that there are a lot of use cases for array selectors. I have been using this on a few projects and I think it would be a good addition to the library.

const list: string[] = ["apple", "pear", "orange"]
const [selectedItems] = createSignal<string[]>(["apple"])
const isSelected = createArraySelector(selectedItems)

<For each={list}>
  {(item) => <li classList={{ active: isSelected(item) }}>{item}</li>}
</For>

Extending with a comparator function (instead of b.includes(a)) to support objects could be possible, but I decided to keep it simple. At that point it might be better to just create a custom selector directly.

@changeset-bot
Copy link

changeset-bot bot commented Sep 24, 2023

⚠️ No Changeset found

Latest commit: 0e5a5bd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@thetarnav thetarnav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an interesting idea to have a package dedicated to the selector primitive. There might be other helpers this could include.
But right now I wouldn't advice anyone to use it with only createArraySelector, especially when it doesn't provide too much value.
Also I'm wondering if instead of exporting a createSelector wrapper, it would be better to just export the (key, list) => list.includes(key) part. Then one could compose it with the selector.

import { arrayIncludes } from "@solid-primitives/selector"

const isItemSelected = createSelector(selectedItems, arrayIncludes)

* </For>
*/
export function createArraySelector<T>(
source: () => Array<T>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should allow readonly arrays too

Suggested change
source: () => Array<T>,
source: () => readonly T[],

*/
export function createArraySelector<T>(
source: () => Array<T>,
options?: { name?: string },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to reuse types from solid for the options (and any other type if possible), to show where it is coming from, and remove the need for updating the types when options in solid change.

@@ -0,0 +1,57 @@
{
"name": "@solid-primitives/selectors",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer selector singular to match the rest of the packages

{(item) => <li classList={{ active: isSelected(item) }}>{item}</li>}
</For>
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding that it doesn't work when mutating the array:
https://playground.solidjs.com/anonymous/bfeda055-2f2c-465e-b438-ea9d671a4eb8

{(item) => <li classList={{ active: isSelected(item) }}>{item}</li>}
</For>
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth showing that the implementation is basically an alias to createSelector with Array.includes

const createArraySelector = source => createSelector(source, (key, list) => list.includes(key))

I don't want anyone to get an impression that it's not possible to do this with createSelector

@thetarnav thetarnav added the Primitive Proposal For discussing a primitive proposition and the API design label Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primitive Proposal For discussing a primitive proposition and the API design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants