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

Framework: Merge our two initialization actions into a single onee #2933

Merged
merged 1 commit into from Oct 9, 2017

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Oct 9, 2017

Just a small refactoring merging our two initialization actions into a single one SETUP_EDITOR. As a follow up we could imagine dropping the SETUP_EDITOR effect entirely, all its behavior is synchronous and could be moved to the reducer, but it serves as a factorization for some processing (parsing).

Testing instructions

  • Ensure the editor loads properly for new and saved posts.

@youknowriad youknowriad self-assigned this Oct 9, 2017

@youknowriad youknowriad requested a review from aduth Oct 9, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 9, 2017

Codecov Report

Merging #2933 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2933   +/-   ##
=======================================
  Coverage   33.92%   33.92%           
=======================================
  Files         193      193           
  Lines        5701     5701           
  Branches     1000     1000           
=======================================
  Hits         1934     1934           
  Misses       3187     3187           
  Partials      580      580
Impacted Files Coverage Δ
editor/effects.js 40% <ø> (ø) ⬆️
editor/index.js 0% <ø> (ø) ⬆️
editor/actions.js 38.88% <ø> (ø) ⬆️

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 c80ac15...5fdd34b. Read the comment docs.

codecov bot commented Oct 9, 2017

Codecov Report

Merging #2933 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2933   +/-   ##
=======================================
  Coverage   33.92%   33.92%           
=======================================
  Files         193      193           
  Lines        5701     5701           
  Branches     1000     1000           
=======================================
  Hits         1934     1934           
  Misses       3187     3187           
  Partials      580      580
Impacted Files Coverage Δ
editor/effects.js 40% <ø> (ø) ⬆️
editor/index.js 0% <ø> (ø) ⬆️
editor/actions.js 38.88% <ø> (ø) ⬆️

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 c80ac15...5fdd34b. Read the comment docs.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 9, 2017

Member

Will take a closer look at this shortly. One concern which was already raised (by @mtias) about how we initialize state for the editor is that effects cause some of the details to be lost by the indirection. Part of this is acknowledged by the nature of side-effects (not necessary to be known how an action is handled), but it's hard to argue against the worry that it's unclear at a glance of editor/index.js what is happening when setting up a post. There could be some credibility to creating distinct setup tasks, even if they're not strictly necessary and could be collapsed into a single action.

Member

aduth commented Oct 9, 2017

Will take a closer look at this shortly. One concern which was already raised (by @mtias) about how we initialize state for the editor is that effects cause some of the details to be lost by the indirection. Part of this is acknowledged by the nature of side-effects (not necessary to be known how an action is handled), but it's hard to argue against the worry that it's unclear at a glance of editor/index.js what is happening when setting up a post. There could be some credibility to creating distinct setup tasks, even if they're not strictly necessary and could be collapsed into a single action.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 9, 2017

Contributor

I share these thoughts, it's not easy to know what's happening on initialization and part of this IMO is caused by the fact that it's spread between several competing actions/effects.

As a follow up we could imagine dropping the SETUP_EDITOR effect entirely, all its behavior is synchronous

Since all of this is synchronous, I think it makes sense to flatten everything into a unique action and move all the behavior to the reducer. It makes things simpler.

Contributor

youknowriad commented Oct 9, 2017

I share these thoughts, it's not easy to know what's happening on initialization and part of this IMO is caused by the fact that it's spread between several competing actions/effects.

As a follow up we could imagine dropping the SETUP_EDITOR effect entirely, all its behavior is synchronous

Since all of this is synchronous, I think it makes sense to flatten everything into a unique action and move all the behavior to the reducer. It makes things simpler.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 9, 2017

Member

Since all of this is synchronous, I think it makes sense to flatten everything into a unique action and move all the behavior to the reducer. It makes things simpler.

Hmm, maybe. Something has always felt off to me about performing heavy work in a reducer. Perhaps this is partly due to concerns in a bundle-split application where a reducer would need to be present for all bundles (and therefore all code it depends on).

Of course, some transformation in a reducer is to be expected, and parsing the input post object could just be seen as one of these transformations. I don't think that an effect need exist based on whether it is synchronous or asynchronous, but I do agree how we're currently using it -- as a transform step between a simple editor initialization and the reducer -- doesn't really fall under the classification of "side effect", and we should move the parsing to one or the other ends of this and drop the effect.

Member

aduth commented Oct 9, 2017

Since all of this is synchronous, I think it makes sense to flatten everything into a unique action and move all the behavior to the reducer. It makes things simpler.

Hmm, maybe. Something has always felt off to me about performing heavy work in a reducer. Perhaps this is partly due to concerns in a bundle-split application where a reducer would need to be present for all bundles (and therefore all code it depends on).

Of course, some transformation in a reducer is to be expected, and parsing the input post object could just be seen as one of these transformations. I don't think that an effect need exist based on whether it is synchronous or asynchronous, but I do agree how we're currently using it -- as a transform step between a simple editor initialization and the reducer -- doesn't really fall under the classification of "side effect", and we should move the parsing to one or the other ends of this and drop the effect.

@aduth

aduth approved these changes Oct 9, 2017

Other conversation notwithstanding, this is a good first step 👍

@youknowriad youknowriad merged commit b3eec60 into master Oct 9, 2017

3 checks passed

codecov/project 33.92% remains the same compared to c80ac15
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/editor-initialization-state branch Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment