Add new API for accessing store in UI components #2800

robertknight · 2020-12-08T12:43:54Z

This PR is a prototype of a new API for accessing the store in UI components which aims to fix several problems of the useStore hook:

Every call to useStore created a bunch of effects and other hooks and added a new subscriber to the store. This adds overhead if a component calls useStore many times.
In the case where useStore is being used to get an action, this overhead is completely unnecessary
There were various ways to misuse useStore which have been come up several times. For example, returning a selector function from a useStore hook instead of calling the selector and returning the result.

The new API takes a different approach:

import { useStoreProxy } from '../store/use-store';

...

function MyComponent() {
  const store = useStoreProxy();
  const currentUser = store.currentUser();
                                                              
  return (
    <div>
      Current user: {currentUser}.
      <button onClick={() => store.logOut()}>Log out</button>
    </div>
  );
}

Here useStoreProxy (~~temporary name, to avoid confusion with useStore~~ Edit: Feedback has been that perhaps this name should stick) returns a proxy for the store which caches the result of selector method calls and checks whether the results of any of those cached calls have changed whenever the store state changes. If so, the cache is invalidated and the component is re-rendered.

Compared to useStore there are several benefits:

Only one store subscriber per component instance
Less boilerplate to interact with the store
Much less overhead for calling actions, and that overhead is only incurred when the action is actually called
Harder to misuse

In order for this API to work, the useStoreProxy makes an assumption about the store API: All the methods that components will use either return something from the store (selectors) but don't change it, or modify the store in some way (actions). These can be distinguished by checking whether the store state changed after calling the method.

This PR implements the useStoreProxy hook and associated tests. Subsequent PRs will switch UI components over to use the new hook. Finally once all components are converted, the old useStore hook and tests will be removed.

codecov · 2020-12-08T12:58:06Z

Codecov Report

Merging #2800 (ff6890d) into master (899a283) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2800      +/-   ##
==========================================
+ Coverage   97.74%   97.75%   +0.01%     
==========================================
  Files         202      202              
  Lines        7678     7723      +45     
  Branches     1700     1707       +7     
==========================================
+ Hits         7505     7550      +45     
  Misses        173      173

Impacted Files	Coverage Δ
src/sidebar/store/use-store.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 899a283...ff6890d. Read the comment docs.

lyzadanger

This approach makes sense and is a good use of Proxy (IMHO). It's clean and generally straightforward, but there is some inherent complexity, so extra commenting is helpful, I think. Look forward to full reviewing when tests are in place (I looked at the implementation of useStoreProxy but ignored the boilerplate changes across the various components; ran eyes over the other couple of commits and they looked fine so far, too).

lyzadanger · 2020-12-08T19:47:48Z

src/sidebar/store/use-store.js

+      wrapped = (...args) => {
+        const cacheEntry = cache.find(entry => entry.matches(prop, args));
+        if (cacheEntry) {
+          return cacheEntry.result;


This threw me at first, and we discussed it verbally. This cache entry's result is reliable because the cache entries are removed/invalidated if any of the cached methods-with-args return different results in the useEffect hook (store.subscribe callback) below. I'm not sure if this would throw a future dev; some commenting might help.

lyzadanger · 2020-12-08T19:48:40Z

src/sidebar/store/use-store.js

+        // to things which no longer exist (for example, an object ID for an object
+        // which has been unloaded). It is assumed that store selector methods are
+        // robust to this.
+        entry => store[entry.method](...entry.args) !== entry.result


This line takes a little energy to unpack, but I can't think of how to do it better; it is self-documenting if you take the time to read it slowly...

lyzadanger · 2020-12-08T19:55:26Z

src/sidebar/store/use-store.js

+        // the store but returns a result, or an action that modifies the state
+        // but does not return a result.
+        const result = original.call(target, ...args);
+        if (result !== undefined) {


A more direct comment here might be helpful: we're not adding cache entries for actions. You address this in the comment above but for some reason it didn't translate clearly for me...

lyzadanger · 2020-12-08T20:00:30Z

src/sidebar/store/use-store.js

+  // relevant store state changes.
+  useEffect(() => {
+    const cleanup = store.subscribe(() => {
+      const isCacheInvalid = cache.some(


Gimme a hand here (running low on time and don't want to unravel useStore internals right now)...does this cache-clearing of all "registered" store selectors for the calling component ultimately result in selectors getting recomputed more often than the previous implementation? The same? Apples and oranges?

It may be slightly more efficient in the case where a re-render needs to happen. Previously we would always re-evaluate every selector used by a component when anything in the store changed (ie. we would re-run all the useStore hook callbacks). We will now evaluate the selector calls in the order they occurred and bail out early if we detect that any are invalid.

Implement a new `useStoreProxy` hook that provides access to read and update the store in UI components. This will replace the existing `useStore` hook. The new hook has a more ergonomic API which should also prevent some of the common mistakes made when using the previous hook. `useStoreProxy` returns an ES proxy for the store. The UI component can use the proxy as if it were using the store directly. The proxy however tracks and caches the results of store selector calls. When the store state changes the proxy checks whether any of the cached calls would return different results and if so invalidates the cache and re-renders the component.

robertknight · 2020-12-09T07:42:39Z

src/sidebar/store/test/use-store-test.js

-    const wrapper = mount(<TestComponent />);
-    assert.equal(wrapper.text(), '10');
-  });
+  // Tests for deprecated `useStore` function.


The diff in the describe('useStore', ... block are just whitespace changes. See the diff without whitespace changes.

robertknight · 2020-12-09T07:43:42Z

src/sidebar/store/test/use-store-test.js

-      },
-    });
-  });
+// Store module for use with `createStore` in tests.


Since the useStoreProxy hook is intended to be used with a store created by our createStore function (as opposed to the one from the redux package) I figured it was better to use such a store in the tests.

This is not the first time that naming "collision" has thrown me a tad.

lyzadanger

This will improve developer ergonomics and clarity, as well as feeling like a steadier solution overall. Good.

There is some necessary complexity here. Most of my comments point out opportunities to increase commenting clarity to help future devs.

lyzadanger · 2020-12-09T14:23:13Z

src/sidebar/store/use-store.js

+  const [, forceUpdate] = useReducer(x => x + 1, 0);
+
+  // Cache of store method calls made by the current UI component and associated
+  // results. This is currently just an array on the assumption that it will


When I first started looking at the draft changes the other day, I wasn't entirely clear on the cache entries on first read: I was worried there might be on entry per call (oy), or perhaps one entry per (method, args, result) set, when in fact there is one entry per (method, args) combination. It may be possible to tweak this comment to reflect that more clearly; especially, because CacheEntry has a results property.

lyzadanger · 2020-12-09T14:24:39Z

src/sidebar/store/use-store.js

+  const cacheRef = useRef(/** @type {CacheEntry[]} */ ([]));
+  const cache = cacheRef.current;
+
+  // Create the wrapper around the store.


Nit: Here and in a couple of other one-liner comments in this module: I'd nix the full stop.

lyzadanger · 2020-12-09T14:29:05Z

src/sidebar/store/use-store.js

+     * @param {typeof store} target
+     * @param {string} prop
+     */
+    const get = (target, prop) => {


I find myself torn between:

All of the implementation here perfectly follows what one is supposed to do with a Proxy, including using consistent reference naming, vs.

A few arguably-superfluous comments would help a future dev comprehend this more quickly;

What do you think?

lyzadanger · 2020-12-09T14:45:43Z

src/sidebar/store/use-store.js

+ * extract data from the store and call actions on it.
+ *
+ * Unlike using the `store` service directly, the wrapper tracks what data from
+ * the store the current component uses and re-renders the component when it


The "it" in this sentence is a little bit ambiguous. Is it accurate to say "re-renders the component when the results of any selectors used changed" or something perhaps a little less clunky?

lyzadanger · 2020-12-09T14:45:57Z

src/sidebar/store/use-store.js

+ * the store the current component uses and re-renders the component when it
+ * changes.
+ *
+ * The returned wrapper has the same API as the store itself.


This is a handy comment: thank you.

lyzadanger · 2020-12-09T15:01:13Z

src/sidebar/store/test/use-store-test.js

-    assert.calledOnce(testStore.subscribe);
-    wrapper.unmount();
-    assert.calledOnce(unsubscribe);
+      assert.ok(proxy);


Can you elaborate on what this is testing?

The subsequent tests—I think—wouldn't pass if this didn't pass, right?

This is just a sanity check that useStoreProxy returned something and that the test helper returned that value. It isn't that important.

lyzadanger · 2020-12-09T15:01:44Z

src/sidebar/store/test/use-store-test.js

-    assert.calledOnce(unsubscribe);
+      assert.ok(proxy);
+
+      // Test proxied selector method.


Good commenting, thanks.

lyzadanger · 2020-12-09T15:03:09Z

src/sidebar/store/test/use-store-test.js

+      });
+    });
+
+    function renderTestComponent() {


Is the return value from this function structured in imitation, or is it just for the needs of these tests?

It is just used as a way to return multiple values, of which most tests only need some components.

lyzadanger · 2020-12-09T15:04:41Z

src/sidebar/store/test/use-store-test.js

+      assert.calledWith(addThingSpy, 'baz');
+    });
+
+    it('proxies non-function properties of the store', () => {


Are there any known current use cases for this, or are you merely illustrating that such a thing would be possible? Should this be commented? Should it (access to non-function properties) be allowed?

We don't currently have a specific need to be able to access non-function properties, so this is just for completeness.

lyzadanger · 2020-12-09T15:08:23Z

src/sidebar/store/test/use-store-test.js

+      getThingSpy.resetHistory();
+
+      proxy.getThing('bar');
+      proxy.getThing('bar');


Do you feel like a test like this sufficiently exercises the path of "the component re-renders and re-invokes its various proxied store selector methods and we can see that those selectors are not re-invoked because their cache entries are still valid"?

It does cover that functionality, but only in a minimal way. I think that's adequate for now but we might want to extend it in future.

Make several improvements following PR review.

robertknight marked this pull request as draft December 8, 2020 12:44

robertknight force-pushed the use-store-proxy branch from ce96327 to 92ce6ee Compare December 8, 2020 12:55

robertknight force-pushed the use-store-proxy branch 2 times, most recently from d821c7e to 870027d Compare December 8, 2020 17:03

lyzadanger reviewed Dec 8, 2020

View reviewed changes

robertknight force-pushed the use-store-proxy branch from 870027d to 923f698 Compare December 9, 2020 07:24

robertknight marked this pull request as ready for review December 9, 2020 07:33

robertknight force-pushed the use-store-proxy branch from 923f698 to 7e38cb9 Compare December 9, 2020 07:41

robertknight commented Dec 9, 2020

View reviewed changes

robertknight requested a review from lyzadanger December 9, 2020 07:48

lyzadanger approved these changes Dec 9, 2020

View reviewed changes

Improve several variable names and comments

ff6890d

Make several improvements following PR review.

robertknight merged commit 5bcdd78 into master Dec 9, 2020

robertknight deleted the use-store-proxy branch December 9, 2020 16:20

robertknight mentioned this pull request Dec 9, 2020

Replace useStore hook with useStoreProxy in UI components #2808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new API for accessing store in UI components #2800

Add new API for accessing store in UI components #2800

robertknight commented Dec 8, 2020 •

edited

Loading

codecov bot commented Dec 8, 2020 •

edited

Loading

lyzadanger left a comment

lyzadanger Dec 8, 2020

lyzadanger Dec 8, 2020

lyzadanger Dec 8, 2020

lyzadanger Dec 8, 2020

robertknight Dec 9, 2020

robertknight Dec 9, 2020

robertknight Dec 9, 2020 •

edited

Loading

lyzadanger Dec 9, 2020

lyzadanger left a comment

lyzadanger Dec 9, 2020

lyzadanger Dec 9, 2020

lyzadanger Dec 9, 2020

lyzadanger Dec 9, 2020

lyzadanger Dec 9, 2020

lyzadanger Dec 9, 2020

robertknight Dec 9, 2020

lyzadanger Dec 9, 2020

lyzadanger Dec 9, 2020

robertknight Dec 9, 2020

lyzadanger Dec 9, 2020

robertknight Dec 9, 2020

lyzadanger Dec 9, 2020

robertknight Dec 9, 2020

Add new API for accessing store in UI components #2800

Add new API for accessing store in UI components #2800

Conversation

robertknight commented Dec 8, 2020 • edited Loading

codecov bot commented Dec 8, 2020 • edited Loading

Codecov Report

lyzadanger left a 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

robertknight Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzadanger left a 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

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

robertknight commented Dec 8, 2020 •

edited

Loading

codecov bot commented Dec 8, 2020 •

edited

Loading

robertknight Dec 9, 2020 •

edited

Loading