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
[labs/redux] Reactive controller for subscribing to a Redux store #4586
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 03bed21 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
The size of lit-html.js and lit-core.min.js are as expected. |
9bd7532
to
7968db3
Compare
dbc5692
to
49dc2c8
Compare
b3be3a5
to
03bed21
Compare
@@ -0,0 +1,2 @@ | |||
/node_modules/ | |||
/dist/ |
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.
who's writing to /dist/
, Vite? Leave a comment?
@@ -0,0 +1,28 @@ | |||
BSD 3-Clause License | |||
|
|||
Copyright (c) 2017 Google LLC. All rights reserved. |
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.
2024?
|
||
### Connect a child component to the provided store | ||
|
||
Note: The component must be a child of the provider above. Attempting to connect without a store provided via context will throw an error. |
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 wonder if you want to mention ContextRoot here...
Note: The component must be a child of the provider above. Attempting to connect without a store provided via context will throw an error. | ||
|
||
#### Using the `Connector` controller | ||
|
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.
Add a short description of the the Connector controller is first?
|
||
The `@dispatch()` decorator takes no arguments. | ||
|
||
Note: Like all the Lit 3 decorators, these decorators will work as either TypeScript experimental decorators or [standard decorators](https://lit.dev/docs/components/decorators/#standard-decorators). When running as standard decorators, `@select()` decorated fields must also have the `accessor` keyword. `@dispatch()` decorated fields do not have this requirement. |
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.
Standard decorators can decorate private fields too.
* | ||
* This controller must be used with a provider that supplies the Redux store | ||
* using the `storeContext` from this package using the [Context | ||
* Protocol](https://github.com/webcomponents-cg/community-protocols/blob/main/proposals/context.md). |
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.
Add link to @lit/context
too
* @example | ||
* | ||
* ```ts | ||
* // `AppStore` type gotten from created Redux store |
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.
Remove "gotten"
this._store = store as S; | ||
}, | ||
}); | ||
if (this._store === undefined) { |
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 this context subscribes, is this error correct here? What if the page is using a ContextRoot? Maybe this should be some kind of debug warning.
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 had that thought too..
I was hoping if it might be possible to kind of force this requirement on users to force the context provider be always set up first because I otherwise we have to consider that without having the store on start up, the selected
value could be undefined and users need to always guard for that.
So if a user selected for an object, they would basically have to always null check any property access to it for their first render. Maybe that's not the worst thing, but I really like the idea that, assuming the user provided the correct store type and a valid selector, they can always expect a certain value to exist and use as such within their component code.
} | ||
|
||
hostConnected() { | ||
new ContextConsumer(this._host, { |
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 think you need to make the consumer in the constructor and hold a reference to it. That's how I usually show examples of controller composition.
The callback in this case will keep the consumer instance alive even after this method exits, which means on a disconnect/reconnect you will create another long-lived consumer, causing a memory leak. This could be noticable for connected components inside repeat()
or cache()
.
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.
Would storing the consumer instance and checking to not re-create work? I think I had some timing issues when instantiating in the constructor because the context value (redux store) would not be immediately available.
Putting this up as draft PR. There's a few more things needed here. Not sure if/when I might be able to get to it.Changes are ready to review.
This PR adds a new
@lit-labs/redux
package and an example project making use of it.It adds a reactive controller for subscribing to a redux store from context and trigger updates for host component when the redux store updates.
It also includes decorators that use the controller above for selecting state, and bringing in a dispatch to the component.
See the README file for the package for more details.
TODO
Connector
with selector function being optional so it can do either select and/or dispatch, like theconnect()
function fromreact-redux
.useSelector
anduseDispatch
hooks fromreact-redux
.