Skip to content
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

Redux refactor #1571

Merged
merged 113 commits into from
Jul 14, 2020
Merged

Redux refactor #1571

merged 113 commits into from
Jul 14, 2020

Conversation

bkmartinjr
Copy link
Contributor

@bkmartinjr bkmartinjr commented Jun 16, 2020

This is a major refactor of the internal reducer state/store. An "annotated matrix" object, which is a lazy data loader/proxy for the server-side is introduced. Components now have responsibility for loading any data they depend upon, and computing any derived state from that data. This is facilitated by the use of react-async.

Fixes #1605


To do list:

  • menubar
  • user annotations (create, edit, delete, etc). Delete dimensions from crossfilter as needed.
  • autosave user annotations
  • invalidate cached annoMatrix data, especially X array which could blow up in size
  • scatterplot
  • undo/redo
  • disable experimental reembedding - deferred for future refactor
  • unit tests:
    • make all old ones work
    • figure out how to mock AnnoMatrix tests server deps (? https://github.com/jefflau/jest-fetch-mock ?)
    • add unit tests for the new labelIndex.isubsetMask() functions
    • rewrite all the "world" oriented tests to use annoMatrix
    • AnnoMatrixCrossfilter
  • smoke tests
  • make sure no use of "world" or "Universe" anywhere:
    • remove unused reducers
    • remove stateManager reducer/world helpers
  • introduce react-async refactoring:
    • graph
    • scatterplot
    • categorical categories
    • categorical value
    • brushable histogram
    • connect async load error handler to error toast UI
  • anno matrix module cleanup:
    • public/private notation
    • move up a directory
    • finalize comments on public API
  • perf validation on large/latent use cases
  • race condition testing
  • fetch throttle with priority queue to make initial load work well for very large datasets

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2020

Codecov Report

Merging #1571 into main will increase coverage by 2.42%.
The diff coverage is 82.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1571      +/-   ##
==========================================
+ Coverage   65.69%   68.12%   +2.42%     
==========================================
  Files         104      115      +11     
  Lines        9352    10023     +671     
  Branches      378      458      +80     
==========================================
+ Hits         6144     6828     +684     
- Misses       3126     3135       +9     
+ Partials       82       60      -22     
Flag Coverage Δ
#frontend 71.53% <82.72%> (+3.41%) ⬆️
#javascript 71.53% <82.72%> (+3.41%) ⬆️
#smokeTest 100.00% <ø> (ø)
#unitTest 71.53% <82.72%> (+3.41%) ⬆️
Impacted Files Coverage Δ
client/src/util/stateManager/annotationsHelpers.js 12.50% <0.00%> (+8.55%) ⬆️
client/src/annoMatrix/fetchHelpers.js 10.00% <10.00%> (ø)
client/src/annoMatrix/middleware.js 12.50% <12.50%> (ø)
client/src/annoMatrix/annoMatrix.js 66.66% <66.66%> (ø)
client/src/annoMatrix/views.js 73.13% <73.13%> (ø)
client/src/annoMatrix/schema.js 74.28% <74.28%> (ø)
client/src/util/stateManager/schemaHelpers.js 72.34% <75.00%> (+55.31%) ⬆️
client/src/annoMatrix/viewCreators.js 80.00% <80.00%> (ø)
...nt/__tests__/util/annoMatrix/serverMocks/routes.js 88.37% <88.37%> (ø)
client/src/annoMatrix/loader.js 94.33% <94.33%> (ø)
... and 56 more

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 13246cb...dff3af9. Read the comment docs.

@bkmartinjr bkmartinjr changed the title [WIP] redux refactor Redux refactor Jul 4, 2020
client/src/components/graph/graph.js Show resolved Hide resolved
client/src/annoMatrix/views.js Outdated Show resolved Hide resolved
client/src/actions/annotation.js Show resolved Hide resolved
client/src/actions/annotation.js Show resolved Hide resolved
client/src/annoMatrix/annoMatrix.js Outdated Show resolved Hide resolved
client/src/annoMatrix/crossfilter.js Outdated Show resolved Hide resolved
client/src/annoMatrix/annoMatrix.js Show resolved Hide resolved
client/src/components/graph/graph.js Outdated Show resolved Hide resolved
client/src/components/graph/graph.js Show resolved Hide resolved
client/src/components/graph/graph.js Outdated Show resolved Hide resolved
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the awesome PR, Bruce! Only partial reviews from a quick glance 😄 Thanks!

client/src/components/graph/graph.js Outdated Show resolved Hide resolved
client/src/components/graph/graph.js Outdated Show resolved Hide resolved
client/src/components/graph/graph.js Outdated Show resolved Hide resolved
client/src/components/menubar/embedding.js Outdated Show resolved Hide resolved
client/src/components/menubar/embedding.js Outdated Show resolved Hide resolved
client/src/components/scatterplot/scatterplot.js Outdated Show resolved Hide resolved
client/src/util/shallowEqual.js Outdated Show resolved Hide resolved
@colinmegill colinmegill self-requested a review July 13, 2020 20:40
Copy link
Member

@seve seve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incremental review :)

client/src/actions/index.js Outdated Show resolved Hide resolved
client/src/components/graph/overlays/centroidLabels.js Outdated Show resolved Hide resolved
client/src/components/graph/overlays/centroidLabels.js Outdated Show resolved Hide resolved
client/src/components/brushableHistogram/index.js Outdated Show resolved Hide resolved
Copy link
Member

@seve seve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit + a couple questions

client/src/components/graph/overlays/centroidLabels.js Outdated Show resolved Hide resolved
client/src/components/graph/overlays/centroidLabels.js Outdated Show resolved Hide resolved
client/src/components/graph/overlays/centroidLabels.js Outdated Show resolved Hide resolved
Copy link
Member

@seve seve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small fix then we're ready to go!

client/src/components/graph/overlays/centroidLabels.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

initial redux refactor
5 participants