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

Refactor: Move revision-fetching into new Simperium middleware #1921

Merged
merged 1 commit into from
Feb 22, 2020

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Feb 20, 2020

As part of our efforts to replace the "wonky app state" we have come across
a few different elements of state that are closer matches to middleware behaviors
than reducer behaviors. Fetching the revisions for a note is one such activity.

Revisions should be gathered whenever we open the revision slider. Previously
when opening the slider we were making two dispatches: one to set the slider
visibility and another to fetch the revisions. Two dispatches are not necessary
and the fetching is purely as side-effect of opening the panel.

In this patch we're creating a new middleware file where we plan to move all
Simperium side-effects eventually. Right now it only watches for appropriate
changes to the revision slider visibility.

Note that the use of middleware has removed the need to chain dispatch calls
as was previously done between noteRevisions and noteRevisionsLoaded. We
now have a reactive response to REVISIONS_TOGGLE and a more declarative
STORE_REVISIONS action.

Because revision state is so closely related to this work we are also moving
the revision state into Redux proper and fixing a bug or two in the process.

  • Previously the revision slider would stay open when selecting another
    note or when creating a new note through the keyboard shortcuts. It did
    this because the React component was imperatively calling to close the
    panel on outsideClick. Now it properly closes when using the keyboard
    shortcuts because the showRevisions reducer is now listening for those
    actions. We could have left it open but I made the decision that it was
    best to prevent confusion to close it. In develop, even though the
    slider stays open it fails to fetch the revisions when changing notes.
    This too is due to the existing imperative and confusing data flows.

  • Previously it was possible to select another note in the time it took
    to download the note revisions. In this race you could potentially see
    the revisions for another note. Now this has been resolved by only
    storing the note revisions if we are still on the same note as when
    they were requested.

Testing

Use the checklist but add extra tests for revisions.

To see the existing bug in production open the revision slide for a note and
then navigate the note list with ++j or the equivalent in non-macOS
systems. Similarly you can navigate in the opposite direction with k instead
of j. Note that the revision slider stays open and also that it doesn't
fetch the revisions of the newly-selected note. Repeat the experiment by creating
a new note with +n.

As part of our efforts to replace the "wonky app state" we have come across
a few different elements of state that are closer matches to middleware behaviors
than reducer behaviors. Fetching the revisions for a note is one such activity.

Revisions should be gathered whenever we open the revision slider. Previously
when opening the slider we were making two dispatches: one to set the slider
visibility and another to fetch the revisions. Two dispatches are not necessary
and the fetching is purely as side-effect of opening the panel.

In this patch we're creating a new middleware file where we plan to move all
Simperium side-effects eventually. Right now it only watches for appropriate
changes to the revision slider visibility.

Note that the use of middleware has removed the need to chain dispatch calls
as was previously done between `noteRevisions` and `noteRevisionsLoaded`. We
now have a reactive response to `REVISIONS_TOGGLE` and a more declarative
`STORE_REVISIONS` action.

Because revision state is so closely related to this work we are also moving
the revision state into Redux proper and fixing a bug or two in the process.

 - Previously the revision slider would stay open when selecting another
   note or when creating a new note through the keyboard shortcuts. It did
   this because the React component was imperatively calling to close the
   panel on `outsideClick`. Now it properly closes when using the keyboard
   shortcuts because the `showRevisions` reducer is now listening for those
   actions. We could have left it open but I made the decision that it was
   best to prevent confusion to close it. In `develop`, even though the
   slider stays open it fails to fetch the revisions when changing notes.
   This too is due to the existing imperative and confusing data flows.

 - Previously it was possible to select another note in the time it took
   to download the note revisions. In this race you could potentially see
   the revisions for another note. Now this has been resolved by only
   storing the note revisions if we are still on the same note as when
   they were requested.
@dmsnell dmsnell requested a review from a team February 20, 2020 20:41
Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

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

It is not nearly as responsive as before but we can work on that going forward.

@dmsnell
Copy link
Member Author

dmsnell commented Feb 22, 2020

It is not nearly as responsive as before

Can you quantify this @belcherj? If something is worse here than in develop then I suspect you've uncovered a bug. There shouldn't be any impact on the responsiveness, or at least I'd be surprised if this did more than add a millisecond to the delay in loading revisions when opening the bar.

@dmsnell dmsnell merged commit 78a057a into develop Feb 22, 2020
@dmsnell dmsnell deleted the simperium/revisions-in-middleware branch February 22, 2020 02:29
@dmsnell dmsnell mentioned this pull request Mar 17, 2020
41 tasks
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.

2 participants