[labs/redux] Reactive controller for subscribing to a Redux store #4586

augustjk · 2024-03-19T01:16:16Z

~~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

Add tests.
JSDoc on user facing exports.
README.
Renamed class to Connector with selector function being optional so it can do either select and/or dispatch, like the connect() function from react-redux.
Decorators for selector and dispatch for better ergonomics over directly using controllers? These would be more akin to useSelector and useDispatch hooks from react-redux.

changeset-bot · 2024-03-19T01:16:20Z

🦋 Changeset detected

Latest commit: 03bed21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@lit-labs/redux	Minor
@lit-examples/redux	Patch

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

github-actions · 2024-03-19T01:16:31Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +7% (-0.26ms - +0.75ms)
^{this-change vs tip-of-tree}

render

this-change: 45.43ms - 47.87ms
this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +4% (-0.93ms - +0.71ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +4% (-0.32ms - +1.50ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +2% (-1.00ms - +0.49ms)
^{this-change vs tip-of-tree}

update

this-change: 456.73ms - 468.57ms
this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +6% (-2.55ms - +2.16ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +3% (-0.86ms - +1.83ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-0.98ms - +5.28ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 466.37ms - 474.81ms
this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-1.70ms - +5.70ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 123.0.6312.122
Sample size: 50
Built by: Benchmarks #6203
Commit: 03bed21

render

Version	Avg time	vs
	45.43ms - 47.87ms	-

update

Version	Avg time	vs
	456.73ms - 468.57ms	-

update-reflect

Version	Avg time	vs
	466.37ms - 474.81ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 123.0.6312.122
Sample size: 30
Built by: Benchmarks #6203
Commit: 03bed21

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	17.88ms - 18.90ms	-	unsure 🔍 -5% - +4% -0.93ms - +0.71ms	unsure 🔍 -5% - +3% -0.86ms - +0.50ms
tip-of-tree tip-of-tree	17.86ms - 19.14ms	unsure 🔍 -4% - +5% -0.71ms - +0.93ms	-	unsure 🔍 -5% - +4% -0.86ms - +0.71ms
previous-release previous-release	18.12ms - 19.03ms	unsure 🔍 -3% - +5% -0.50ms - +0.86ms	unsure 🔍 -4% - +5% -0.71ms - +0.86ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	36.78ms - 39.94ms	-	unsure 🔍 -7% - +6% -2.55ms - +2.16ms	unsure 🔍 -9% - +3% -3.41ms - +1.29ms
tip-of-tree tip-of-tree	36.81ms - 40.31ms	unsure 🔍 -6% - +7% -2.16ms - +2.55ms	-	unsure 🔍 -8% - +4% -3.33ms - +1.61ms
previous-release previous-release	37.68ms - 41.16ms	unsure 🔍 -3% - +9% -1.29ms - +3.41ms	unsure 🔍 -4% - +9% -1.61ms - +3.33ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.43ms - 11.09ms	-	unsure 🔍 -3% - +7% -0.26ms - +0.75ms	unsure 🔍 -6% - +2% -0.64ms - +0.26ms
tip-of-tree tip-of-tree	10.13ms - 10.89ms	unsure 🔍 -7% - +2% -0.75ms - +0.26ms	-	unsure 🔍 -8% - +0% -0.92ms - +0.05ms
previous-release previous-release	10.65ms - 11.25ms	unsure 🔍 -2% - +6% -0.26ms - +0.64ms	unsure 🔍 -1% - +9% -0.05ms - +0.92ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 123.0.6312.122
Sample size: 50
Built by: Benchmarks #6203
Commit: 03bed21

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.68ms - 35.22ms	-	unsure 🔍 -1% - +4% -0.32ms - +1.50ms	unsure 🔍 -1% - +5% -0.23ms - +1.60ms
tip-of-tree tip-of-tree	33.37ms - 34.35ms	unsure 🔍 -4% - +1% -1.50ms - +0.32ms	-	unsure 🔍 -2% - +2% -0.60ms - +0.80ms
previous-release previous-release	33.26ms - 34.26ms	unsure 🔍 -5% - +1% -1.60ms - +0.23ms	unsure 🔍 -2% - +2% -0.80ms - +0.60ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	66.51ms - 68.50ms	-	unsure 🔍 -1% - +3% -0.86ms - +1.83ms	unsure 🔍 -0% - +3% -0.06ms - +2.12ms
tip-of-tree tip-of-tree	66.12ms - 67.93ms	unsure 🔍 -3% - +1% -1.83ms - +0.86ms	-	unsure 🔍 -1% - +2% -0.47ms - +1.56ms
previous-release previous-release	66.03ms - 66.93ms	unsure 🔍 -3% - +0% -2.12ms - +0.06ms	unsure 🔍 -2% - +1% -1.56ms - +0.47ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 123.0.6312.122
Sample size: 50
Built by: Benchmarks #6203
Commit: 03bed21

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	29.72ms - 30.45ms	-	unsure 🔍 -3% - +2% -1.00ms - +0.49ms	unsure 🔍 -1% - +3% -0.25ms - +0.77ms
tip-of-tree tip-of-tree	29.70ms - 30.98ms	unsure 🔍 -2% - +3% -0.49ms - +1.00ms	-	unsure 🔍 -1% - +4% -0.22ms - +1.25ms
previous-release previous-release	29.47ms - 30.18ms	unsure 🔍 -3% - +1% -0.77ms - +0.25ms	unsure 🔍 -4% - +1% -1.25ms - +0.22ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	462.29ms - 467.70ms	-	unsure 🔍 -0% - +1% -0.98ms - +5.28ms	unsure 🔍 -0% - +1% -1.15ms - +6.02ms
tip-of-tree tip-of-tree	461.27ms - 464.43ms	unsure 🔍 -1% - +0% -5.28ms - +0.98ms	-	unsure 🔍 -1% - +1% -2.55ms - +3.12ms
previous-release previous-release	460.21ms - 464.92ms	unsure 🔍 -1% - +0% -6.02ms - +1.15ms	unsure 🔍 -1% - +1% -3.12ms - +2.55ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	503.40ms - 509.49ms	-	unsure 🔍 -0% - +1% -1.70ms - +5.70ms	unsure 🔍 -0% - +1% -2.15ms - +6.17ms
tip-of-tree tip-of-tree	502.34ms - 506.55ms	unsure 🔍 -1% - +0% -5.70ms - +1.70ms	-	unsure 🔍 -1% - +1% -3.52ms - +3.54ms
previous-release previous-release	501.60ms - 507.27ms	unsure 🔍 -1% - +0% -6.17ms - +2.15ms	unsure 🔍 -1% - +1% -3.54ms - +3.52ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2024-03-19T01:18:29Z

The size of lit-html.js and lit-core.min.js are as expected.

packages/labs/redux/src/lib/selector.ts

justinfagnani · 2024-04-17T18:05:30Z

examples/redux/.gitignore

@@ -0,0 +1,2 @@
+/node_modules/
+/dist/


who's writing to /dist/, Vite? Leave a comment?

justinfagnani · 2024-04-17T18:08:09Z

packages/labs/redux/LICENSE

@@ -0,0 +1,28 @@
+BSD 3-Clause License
+
+Copyright (c) 2017 Google LLC. All rights reserved.


justinfagnani · 2024-04-17T18:10:16Z

packages/labs/redux/README.md

+
+### 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.


I wonder if you want to mention ContextRoot here...

justinfagnani · 2024-04-17T18:11:01Z

packages/labs/redux/README.md

+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
+


Add a short description of the the Connector controller is first?

justinfagnani · 2024-04-17T18:16:19Z

packages/labs/redux/README.md

+
+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.


Standard decorators can decorate private fields too.

justinfagnani · 2024-04-17T18:18:14Z

packages/labs/redux/src/lib/connector.ts

+ *
+ * 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).


Add link to @lit/context too

justinfagnani · 2024-04-17T18:19:06Z

packages/labs/redux/src/lib/connector.ts

+   * @example
+   *
+   * ```ts
+   * // `AppStore` type gotten from created Redux store


Remove "gotten"

justinfagnani · 2024-04-17T18:20:36Z

packages/labs/redux/src/lib/connector.ts

+        this._store = store as S;
+      },
+    });
+    if (this._store === undefined) {


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.

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.

justinfagnani · 2024-04-17T18:23:42Z

packages/labs/redux/src/lib/connector.ts

+  }
+
+  hostConnected() {
+    new ContextConsumer(this._host, {


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().

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.

augustjk requested a review from kevinpschaaf as a code owner March 19, 2024 01:16

augustjk marked this pull request as draft March 19, 2024 22:31

augustjk force-pushed the redux-controller branch from 9bd7532 to 7968db3 Compare March 20, 2024 01:35

augustjk force-pushed the redux-controller branch from dbc5692 to 49dc2c8 Compare April 10, 2024 23:12

augustjk marked this pull request as ready for review April 10, 2024 23:18

augustjk requested a review from justinfagnani April 10, 2024 23:18

augustjk added 13 commits April 17, 2024 09:43

Initial labs/redux setup

32643be

Add redux example project

44cb8e0

Rename Selector to Connector and allow selector to be optional

37468f0

Add select and dispatch decorators

a7191d0

WIP testing

ee8751f

Tests for equality and connector

8d75966

Add decorator tests

6565c88

Add withStoreType test

83906a3

Add jsdoc for exported members

df0a049

Update readme

f3fd3a6

Add changeset and ready for publish

2c07343

Add build and test script to monorepo root

229c085

Add missing wireit deps

03bed21

augustjk force-pushed the redux-controller branch from b3be3a5 to 03bed21 Compare April 17, 2024 16:43

justinfagnani requested changes Apr 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[labs/redux] Reactive controller for subscribing to a Redux store #4586

[labs/redux] Reactive controller for subscribing to a Redux store #4586

augustjk commented Mar 19, 2024 •

edited

changeset-bot bot commented Mar 19, 2024 •

edited

github-actions bot commented Mar 19, 2024 •

edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Mar 19, 2024

justinfagnani Apr 17, 2024

justinfagnani Apr 17, 2024

justinfagnani Apr 17, 2024

justinfagnani Apr 17, 2024

justinfagnani Apr 17, 2024

justinfagnani Apr 17, 2024

justinfagnani Apr 17, 2024

justinfagnani Apr 17, 2024

augustjk Apr 24, 2024

justinfagnani Apr 17, 2024

augustjk Apr 24, 2024

		@@ -0,0 +1,28 @@
		BSD 3-Clause License

		Copyright (c) 2017 Google LLC. All rights reserved.


		### 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.

		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


		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.

		@@ -0,0 +1,2 @@
		/node_modules/
		/dist/

[labs/redux] Reactive controller for subscribing to a Redux store #4586

Are you sure you want to change the base?

[labs/redux] Reactive controller for subscribing to a Redux store #4586

Conversation

augustjk commented Mar 19, 2024 • edited

TODO

changeset-bot bot commented Mar 19, 2024 • edited

🦋 Changeset detected

github-actions bot commented Mar 19, 2024 • edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustjk commented Mar 19, 2024 •

edited

changeset-bot bot commented Mar 19, 2024 •

edited

github-actions bot commented Mar 19, 2024 •

edited