Split selection into two store modules and refactor complex rootSelectors
#2787
Conversation
lyzadanger
force-pushed
the
filter-store
branch
from
565ceaa to
1ced5ea
Compare
Codecov Report
@@ Coverage Diff @@
## master #2787 +/- ##
=======================================
Coverage 97.74% 97.74%
=======================================
Files 202 203 +1
Lines 7664 7674 +10
Branches 1694 1694
=======================================
+ Hits 7491 7501 +10
Misses 173 173
Continue to review full report at Codecov.
|
|
|
||
| function init(settings) { | ||
| return { | ||
| filters: {}, |
There was a problem hiding this comment.
Sorry if this is the dumbest question yet, but what is "filters" for?
There was a problem hiding this comment.
A most not-at-ALL-dumb question! I entirely forgot to explain!
I've prototyped some filtering for the Notebook on another branch. This current work you see here is to prepare the path for implementing that type of filtering. The imminent features will make use of this object.
Right now, it does precisely nothing!
It could be argued that this object shouldn't even be in this changeset. I'll see how easy it is to remove for now.
lyzadanger
force-pushed
the
filter-store
branch
from
1ced5ea to
dbe8b73
Compare
|
I assigned myself at Lyza's request to take another look over a few details. See https://hypothes-is.slack.com/archives/C1M8NH76X/p1607357690172800. |
There was a problem hiding this comment.
Hi Lyza,
I've had a look over this. At a high level the changes look good. I did flag an issue about the overhead added by calling useStore many times within a single component, but I think we can address that separately.
One issue I did find is that the change to the way threadAnnotations is called has broken its memoization. Every time useRootThread is called it builds a new thread. See the test case I added in the comments.
| /** | ||
| * @typedef Focus | ||
| * @prop {boolean} configured - Focus config contains valid `user` and | ||
| * is good to go |
There was a problem hiding this comment.
Could the configured state be derived from other state? If so, it may be preferable to eliminate it to prevent issues where its value is inconsistent with the rest of the state (eg. configured is true but some other state is missing).
If it is not possible to derive it, it would be useful to clarify here why not.
There was a problem hiding this comment.
Permission to address this in next change set? filters will be touched immediately again for implementing a user filter in the notebook, and this is a pre-existing issue (and this diff is hugemongous).
The `selection` store module has become overlong and its responsibilities aren't clear. We know we'll be adding some more filtering capabilities to the app in the next short while, and that would make `selection` even more complex and heavy. Split into two store modules: `selection` and `filters`. Temporarily re-implement `rootSelector`s that are needed for generating thread and filter state for components.
Refactor the computation of "filter state", that is, all of the store state that impacts what constitutes applied filters on annotations.
Eliminate the enormous `threadState` `rootSelector` and re-implement as multiple selectors. Refactor `useRootThread` and threading integration tests to account for these changes.
Refactor selectors for evaluating `hasAppliedFilter` to remove cross-module state dependencies.
Remove unused selector and tidy up reference ordering
Also convert `selectionState` to use `createSelector` to avoid it returning a different reference on each call.
lyzadanger
force-pushed
the
filter-store
branch
from
6d01560 to
dd36aa3
Compare
Motivation for these changes: we're about to add on to filtering capabilities in the app and the
selectionmodule has long been over-stretched and its responsibilities unclear. Managing yet more filtering in this single module seemed unwise. Also, there was some complex logic encapsulated in somerootSelectors for the benefit of some components and hooks that were smelly: they were causing store modules (and their states) to be too coupled.This PR:
selectioninto two modules:selectionandfiltersrootSelectorsthreadState,filterStateandhasAppliedFilterin exchange for smaller supporting selectors to accomplish those computations. Components (FilterStatus,SidebarView) and a hook (useRootThread) now have more logic in them to munge these state objects together, instead of relying on the monsterrootSelectors.useRootThreadhook instead of callingthreadAnnotationsdirectly.This is a large diff. I'll add copious commenting and point out some large swaths that aren't new code, just relocated code. It may also be useful to walk through the commits.
Supports hypothesis/product-backlog#1161