-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat(stash): add useStash and improve other helpers #3320
Conversation
🦋 Changeset detectedLatest commit: 08c6d7a The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
05a6731
to
2fcf475
Compare
export type GetRecordArgs< | ||
table extends Table = Table, | ||
defaultValue extends Omit<TableRecord<table>, keyof Key<table>> | undefined = undefined, | ||
> = propwiseXor<{ stash: Stash }, { state: State }> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it's still useful to have both variants (with stash
or with state
) or should we just change all actions to take in state
for simplicity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these two actions, I think it's useful to have both and not much work to maintain both.
The other actions don't make as much sense, like setRecords
needs to have the original stash to mutate state + notify.
Are there other actions that could benefit from having a state
variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing on my mind is that we'll likely have a corresponding useRecord
and useRecords
hooks to mirror the state variant of getRecord
and getRecords
as shorthands for common usage in the React context:
useStash(stash, (state) => getRecord({ state, table, key })
// to
useRecord({ stash, table, key })
The nice things about building with useRecord
and useRecords
hooks instead of useStash
directly is that you avoid the foot gun that is infinite rendering due to unstable return values of a selector without also specifying an equality function - this can all be abstracted away.
No description provided.