-
-
Notifications
You must be signed in to change notification settings - Fork 119
Add createModel API for reactive state containers
#812
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 6bc1669 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
✅ Deploy Preview for preact-signals-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Size Change: +1.22 kB (+0.78%) Total Size: 157 kB
ℹ️ View Unchanged
|
| }; | ||
| }); | ||
|
|
||
| function useModel<TModel>(constructModel: () => Model<TModel>): Model<TModel> { |
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.
This hook will get added to the Preact & React adapters in a follow up PR
| styled.style.left = rect.left + scrollX + "px"; | ||
| styled.style.top = rect.top + scrollY + "px"; |
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.
Little fix for the render flashing to take into account a scrolled window. Ran into this with the model todo app.
…t & React adapters
|
|
||
| //#endregion Effect | ||
|
|
||
| //#region Action |
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 love the regions, we should be good to export this from here because it's a tree-shakeable package
| ? TModel[Key] | ||
| : TModel[Key] extends object | ||
| ? ValidateModel<TModel[Key]> | ||
| : `Property ${Key extends string ? `'${Key}' ` : ""}is not a Signal, Action, or an object that contains only Signals and Actions.`; |
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.
Should we allow for primitive types here to allow our users to leverage getters or are we drawing that out completely?
Added debugData to the state for debugging purposes in the demo.
Format debug data output for better readability.
Summary
This PR adds
createModelandactionto@preact/signals-core, providing a structured way to build reactive state containers that encapsulate signals, computed values, effects, and actions.Key features:
Symbol.disposeDesign decisions
No classes or reflection
The implementation avoids using ES classes internally. Using a class would require reflecting onto a class's constructor and the current signals implementation avoids reflection and proxies, so this follows a similar design philosophy. A class-based API could be built on top of this primitive, like so (shoutout @developit for this neat little hack):
Using
newto instantiate modelsThe public types require using
newto instantiate models. This helps disambiguate the factory function passed intocreateModelfrom the returned constructor. It's easier to explain that "createModelaccepts a factory and returns a class" than "createModelaccepts a factory and returns a factory."In other words, this:
is easier to understand than:
Using
newalso communicates that each call creates a fresh instance with independent state.Internally,
createModelreturns a plain function that can be called withoutnewfor simplicity, but the public types enforcenewfor clarity.Automatically capture effects implement a dispose function
Effects declared inside a model's factory function are captured by
createModelin order to automatically implement a dispose function on the model (exposed as[Symbol.dispose]). This design avoids models needing to manually wire up effect dispose from nested models to the model interface.Factory functions should NOT return
disposefunctionsIf a model needs to run custom logic when it is diposed (that may not be related to signals), it should not return a
dispose()or[Symbol.dispose]. When composing models, this dispose function isn't guarenteed to get called as parent models would need to know that your model has a dispose and manually wire it up.Instead for custom cleanup logic, the recommended pattern is to declare an effect with no signal dependencies that returns a cleanup function that runs the desired cleanup logic. (see "Dispose pattern" below).
Recommended patterns
Explicit readonly pattern
Declare your model interface explicitly and use
ReadonlySignalfor signals that should only be modified through actions. This ensures only actions can modify signals, giving you better insight and control over state changes:Dispose pattern
Generally, if you delcare an effect that has cleanup logic, that cleanup logic will before each execution of the effect function (aka whenever the signals your effect relies on update).
However, if you have cleanup logic that needs to run on model dispose that doesn't depend on signals, define an effect that uses no signals but returns your cleanup function. This mirrors the
useEffect(() => { return cleanup }, [])pattern in React:This pattern is recommended for custom dispose behavior because it allows models to compose naturally - nested models will have their effects cleaned up automatically without manually wiring up dispose functions.
Open Questions
createModelbe in its own package? It would require accessing Effect internals to observe effect creation.#regionmarkers? They produce helpful headings in VSCode's scrollbar preview. Happy to remove them if people don't like them.Future work
useModelhook to Preact & React adapters