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

Framework: Move store files into state directory #3030

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 16, 2017

Iterative progress toward #3012

This pull request seeks to move editor state behaviors into a new state directory. This directory will encompass all state behaviors per the modularity approach proposed in #3012. The changes included merely move the state directory and update all references therein. I debated whether subsequent module splitting should be included in these changes, but decided the potential for merge conflicts was too high to sustain.

Testing instructions:

Verify that the editor builds and runs without any errors.

Follow-up Tasks:

We should be careful to find and update any current branches / pull requests which add to or reference state after these changes are merged.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Oct 16, 2017
@codecov
Copy link

codecov bot commented Oct 16, 2017

Codecov Report

Merging #3030 into master will decrease coverage by <.01%.
The diff coverage is 79.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3030      +/-   ##
==========================================
- Coverage   35.15%   35.14%   -0.01%     
==========================================
  Files         265      277      +12     
  Lines        6721     6720       -1     
  Branches     1216     1216              
==========================================
- Hits         2363     2362       -1     
  Misses       3683     3683              
  Partials      675      675
Impacted Files Coverage Δ
...tor/components/meta-boxes/meta-boxes-area/index.js 0% <ø> (ø) ⬆️
...omponents/block-settings-menu/block-mode-toggle.js 44.44% <ø> (ø) ⬆️
editor/components/post-last-revision/index.js 0% <ø> (ø) ⬆️
editor/components/page-attributes/index.js 77.77% <ø> (ø) ⬆️
editor/components/post-publish-button/index.js 85% <ø> (ø) ⬆️
editor/components/post-publish-button/label.js 83.33% <ø> (ø) ⬆️
editor/components/autosave-monitor/index.js 50% <ø> (ø) ⬆️
editor/components/post-author/index.js 83.33% <ø> (ø) ⬆️
editor/components/post-excerpt/index.js 0% <ø> (ø) ⬆️
editor/components/post-last-revision/check.js 0% <ø> (ø) ⬆️
... and 103 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 ec09a87...4fa0f08. Read the comment docs.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gziolo
Copy link
Member

gziolo commented Oct 18, 2017

Merge conflicts need to be resolved and it should be good to go 👍

@aduth
Copy link
Member Author

aduth commented Oct 18, 2017

I'll plan to land this one after the next release.

@aduth
Copy link
Member Author

aduth commented Nov 16, 2017

Rebased and... well, decided to adopt the Ducks pattern here and now. This is now a monster of a pull request and we should decide sooner than later if we want to move on it, as conflicts will become increasingly likely. Its only saving grace is that while I split features, I kept a master reference of selectors and actions from which to import. This may end up being more convenient anyways, particularly for when we decide to expose selectors for plugin usage. We can obviously decide to move away from this if desired. Similarly, I did not do anything to touch effects quite yet, and will leave for future changes.

Ultimately the Ducks modules ended up being:

  • blockInsertionPoint
  • blockSelection
  • blocksMode
  • currentPost
  • editor
  • hoveredBlock
  • isTyping
  • metaBoxes
  • notices
  • panel
  • preferences
  • reusableBlocks
  • saving
  • ui

@aduth aduth requested a review from mtias November 16, 2017 22:01
@aduth aduth force-pushed the move/editor-state branch from 9d4feec to 5993dd7 Compare November 17, 2017 13:11
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I'm not totally inside the motivations of this change. But I checked the code and the pattern applied in this refactor seems fine, I think this makes our codebase more scalable and easier to work in parallel in different features. I did an extensive set of tests in this branch and I did not found a regression so I'm approving it
👍 Nice work handling a huge change like this!

A fear I have is with time we may create huge "ducks" and even bigger "ducks" test files. Would a pattern like editor/state/meta-boxes/{actions, reducer, selectors}.js instead of editor/state/meta-boxes.js following the same rules in the "ducks" pattern but with artifacts separated in files be an option? I think this pattern is similar to what happens in wp-calypso.

@aduth
Copy link
Member Author

aduth commented Nov 17, 2017

Would a pattern [...] following the same rules in the "ducks" pattern but with artifacts separated in files be an option? I think this pattern is similar to what happens in wp-calypso.

Yeah, I never had an initial appreciation for how similar Ducks is to the pattern we introduced to Calypso, but in implementing it here, there is certainly significant overlap, with the main difference being that Ducks places more emphasis on the reducer as the default export of a module (maybe equivalent to a {subject}/index.js). Personally I am open to this, but it does increase the sprawl of state, and wouldn't be too difficult to introduce later if we wanted (at least imports could stay the same).

@aduth aduth requested a review from dmsnell November 17, 2017 16:49
@gziolo
Copy link
Member

gziolo commented Nov 20, 2017

I had to do some research what Ducks pattern is, but as soon as I learned about it I'm fine with the proposed approach. It also seems to be reasonable to keep two global entry points for actions and selectors. It is much easier to have one place to import things that scan all subfolders for every method. It also ensures that we won't use the same names for different things. Let's rebase and proceed with this one. Next step would be to find a way to make Redux state extensible :shipit:

@aduth aduth force-pushed the move/editor-state branch from 5993dd7 to 4fa0f08 Compare November 20, 2017 14:01
@aduth
Copy link
Member Author

aduth commented Nov 20, 2017

I'll try to rebase to keep this up-to-date, but I'm beginning to question some of the value here. In particular, subject-based selectors can be confusing to implement, since something like getDocumentTitle doesn't fit nicely into an existing subject (implemented in state/ui without any reducer of its own) and getEditedPostValue uses values from multiple state subtrees (both state.editor and state.currentPost). I think we'd also want to settle on whether a consumer would want to import from state/selectors or the subject-specific state/foo.

@gziolo
Copy link
Member

gziolo commented Nov 20, 2017

In case of selectors I'd be personally happy about having them all in one file as it is at the moment. The file itself can grow big with the time, but we can fix that later. Less choice for the consumers of Redux state equals more consistent way of importing things. We can decide later what to do about actions as it might be a better idea to go the opposite way and encourage to import directly from the namespace.

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

No idea how to even begin reviewing this; sorry 🤷‍♂️

I'm mostly dispassionate about it though I encourage grouping functions into fewer files just because of issues with JavaScript.

Please let me know if there are specific ways I can be of assistance here!

@gziolo
Copy link
Member

gziolo commented Jan 10, 2018

I think it's no longer relevant and a bit outdated :)

@aduth
Copy link
Member Author

aduth commented Jan 10, 2018

I think it's no longer relevant and a bit outdated :)

Indeed!

The ducks pattern is still interesting, though added equal parts clarity and confusion; clarity in creating separation where it clearly exists, confusion in things such as selectors or effects which operate on state as a whole.

@aduth aduth closed this Jan 10, 2018
@aduth aduth deleted the move/editor-state branch January 10, 2018 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants