-
Notifications
You must be signed in to change notification settings - Fork 7
feat: add setAtom bound to the state manager #57
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
Conversation
packages/@lwc/state/src/types.ts
Outdated
@@ -52,5 +52,6 @@ export type DefineState = < | |||
computed: MakeComputed, | |||
update: MakeUpdate, |
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.
can we do something to let consumers know that update
is deprecated?
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.
since the package version is 0.x.y
i was thinking about removing the update functionality entirely (working on it in a different PR).
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.
created https://github.com/salesforce/lightning-labs/compare/main...jodarove:lightning-labs:jodarove/remove-update-proposal?expand=1 (contains also this changes) (or this pr against this branch to show the isolated changes)
@@ -52,5 +52,6 @@ export type DefineState = < | |||
computed: MakeComputed, | |||
update: MakeUpdate, | |||
fromContext: MakeContextHook<ContextShape>, | |||
setAtom: <T>(a: Signal<T>, newValue: T) => void, |
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.
I'd like to rethink the atom
/setAtom
names at some point & see if we can come up with something that has more immediate / imperative connotations. Doesn't need to be in this PR, though.
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.
sure thing, let's brainstorm on it!
f92226b
to
56fa5fa
Compare
Description
This PR proposes the addition of a new API for State Managers:
setAtom
, which allows state manager authors to directly update atom values without the need to use anupdate
function. The API limits which atoms can be updated to only those that are created within thedefineState
function.Before this PR:
After this PR:
Things to notice here:
.value
because we are directly using the atom.increment
is executed within, for example aneffect
, this this access will be register for that effect; therefore whenevercount.value
changes, the effect will be executed.Followup proposal: can we remove the
update
function?🟢 No breaking change