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 to remove usage of post related effects in packages/editor. #13716

Merged
merged 32 commits into from
Feb 28, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7e1d857
add controls for fetch, select, and dispatch
nerrad Feb 7, 2019
77a812c
move _all_ constants into the constants file for better organization
nerrad Feb 7, 2019
ae3f60b
establish file structure, begin refactoring post effects/actions
nerrad Feb 7, 2019
a673b8e
Implement trashPost action-generator
nerrad Feb 8, 2019
25c74c2
fix import jsdocs
nerrad Feb 8, 2019
54fbc38
update controls to use new createRegistryControl interface
nerrad Feb 8, 2019
455d6a8
add action generator for refreshing the current post
nerrad Feb 8, 2019
5d246e5
add tests for new code
nerrad Feb 11, 2019
0008288
fix broken tests
nerrad Feb 11, 2019
de8e6d1
don’t continue with refreshPost if there’s no current post yet
nerrad Feb 11, 2019
64216f4
remove unnecessary bail
nerrad Feb 11, 2019
f80fae4
add changelog entry
nerrad Feb 11, 2019
97b1ed5
fix lint failure for import order
nerrad Feb 12, 2019
9bce642
fix incorrect method used for autosave requests
nerrad Feb 12, 2019
36f962d
remove another wayward period.
nerrad Feb 12, 2019
d69599f
Add resolveSelect control and implement
nerrad Feb 19, 2019
4cba06a
refactor to condense code and update tests.
nerrad Feb 22, 2019
b3beb7f
update docs
nerrad Feb 22, 2019
651216d
fix doc blocks
nerrad Feb 27, 2019
a455781
fix jsdocs for param object properties
nerrad Feb 27, 2019
1a4fc0c
simplify import statement
nerrad Feb 27, 2019
49f5af6
utilize constants from constants file
nerrad Feb 27, 2019
a41039c
remove unused constants (now in block-editor)
nerrad Feb 27, 2019
2c14df4
simplify import
nerrad Feb 27, 2019
f66af80
add doc blocks for control actions
nerrad Feb 27, 2019
5568ac3
update docs
nerrad Feb 27, 2019
6d74643
make sure test is importing used constant
nerrad Feb 27, 2019
ff70f2b
fix doc block spacing issues
nerrad Feb 28, 2019
1c11a5b
Swith constant name from `MODULE_KEY` to `STORE_KEY`
nerrad Feb 28, 2019
4cc4015
remove use of generatorActions default import ans switch to dispatch …
nerrad Feb 28, 2019
0ba7d0e
doc changes
nerrad Feb 28, 2019
c17179e
Fix docs spacing
aduth Feb 28, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add tests for new code
- also removes old code that’s been replaced and fixes up imports
  • Loading branch information
nerrad committed Feb 28, 2019
commit 5d246e544107b18df7fed7b548638703c0c63392
49 changes: 45 additions & 4 deletions packages/editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,52 @@ export function resetPost( post ) {

/**
* Returns an action object used in signalling that the latest autosave of the
* post has been received, by initialization or autosave.
* specified client ID has been selected, optionally accepting a position
* value reflecting its selection directionality. An initialPosition of -1
* reflects a reverse selection.
*
* @param {Object} post Autosave post object.
* @param {string} clientId Block client ID.
* @param {?number} initialPosition Optional initial position. Pass as -1 to
* reflect reverse selection.
*
* @return {Object} Action object.
*/
export function selectBlock( clientId, initialPosition = null ) {
return {
type: 'SELECT_BLOCK',
initialPosition,
clientId,
};
}

export function startMultiSelect() {
return {
type: 'START_MULTI_SELECT',
};
}

export function stopMultiSelect() {
return {
type: 'STOP_MULTI_SELECT',
};
}

export function multiSelect( start, end ) {
return {
type: 'MULTI_SELECT',
start,
end,
};
}

export function clearSelectedBlock() {
return {
type: 'CLEAR_SELECTED_BLOCK',
};
}

/**
* Returns an action object that enables or disables block selection.
*
nerrad marked this conversation as resolved.
Show resolved Hide resolved
* @return {Object} Action object.
*/
Expand Down Expand Up @@ -125,8 +168,6 @@ export function refreshPost() {
export function trashPost( postId, postType ) {
return {
type: 'TRASH_POST',
postId,
postType,
};
}

Expand Down
2 changes: 2 additions & 0 deletions packages/editor/src/store/actions/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './post-actions';
Copy link
Member

Choose a reason for hiding this comment

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

Is there a compelling reason to segregate the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean among the post actions? or do you mean specifically having "post" type actions and eventually "block" type actions?

If the former, I find it easier to know where to go for general actions as opposed to action generators. It also aids with splitting up tests to cover the specific things.

Copy link
Member

Choose a reason for hiding this comment

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

I find it easier to know where to go for general actions as opposed to action generators.

It doesn't seem to me that it's something someone should need to distinguish. I think a current reality is that we have spaghetti generators, but they're all just "actions (dispatchers)", ideally returning or yielding simple action objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm probably ignorant of potential reasons why we wouldn't want to split up things in this case. Are there technical/performance/build reasons why its good to avoid splitting up files like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually try to logically split files up into smaller components when I can because:

a. it helps with finding things quicker when I'm just looking for a "class" of logic
b. it makes for smaller number of lines in a file, so less scrolling.
c. My IDE built-in linting etc doesn't get bogged down as much due to larger files.

But as I already mentioned, I'm aware I may be ignorant of good reasons for not splitting things up as I did here.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, no directives. Just surfacing my own opinions stemming from the initial concern / probe for rationale of the split. It seems there's at least a second opinion in your favor from @youknowriad , but I'd go on record to disagree with the "domain"-based split (it wasn't but an hour after my previous complaint about the effects structuring that I wrongly navigated to effects/posts.js as my first destination in pursuit of this snippet of code).

To your question, yes, I'd see it all just being in a single actions.js file.

There's an interesting overlap between this discussion and some older ones like in #3030, where I think ultimately it was the introduction of @wordpress/data and its store-namespacing that served as the alternative to this problem of files becoming large and unwieldy, where the separation was by stores (modules), not by separate files within a store.

Copy link
Contributor

Choose a reason for hiding this comment

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

actions.js is better if we had one file but if the file grows indefinitely, it becomes hard to navigate the file. maybe there's a middleground to find at some point.

I also said to @nerrad that in general I'm not very opinionated on these matters :)

Copy link
Contributor Author

@nerrad nerrad Feb 14, 2019

Choose a reason for hiding this comment

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

but if the file grows indefinitely, it becomes hard to navigate the file.

Ya this is one criteria I use when deciding whether to split up a file. For example just with the post actions here, combined, they would be 487 lines. That's before adding in all the block and editor related actions.

as my first destination in pursuit of this snippet of code).

@aduth my plan was to have editor-actions and block-actions files that would accompany the post-actions. That snippet is found within the SETUP_EDITOR editor effect that would have landed in editor-actions (under the original plan).

Would a middle ground here be to not split between action generators and action functions and just have actions/post.js, actions/editor.js, and actions/blocks.js? That might also be useful for aiding future refactors where some (or all) of the block specific actions are moved elsewhere.

Copy link
Contributor Author

@nerrad nerrad Feb 14, 2019

Choose a reason for hiding this comment

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

Keep in mind, I'm not only considering the line count for a combined actions file, but also the line count that would be exponentially larger for the corresponding tests file. Currently combining actions/test/post.js and actions/test/post-generators.js into one file results in 759 lines. As soon as a file gets over roughly 500 lines I find it starts to get unwieldy. I imagine the line count for a combined file of editor, post, and block actions tests will start to go over 2000 easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an interesting overlap between this discussion and some older ones like in #3030, where I think ultimately it was the introduction of @wordpress/data and its store-namespacing that served as the alternative to this problem of files becoming large and unwieldy, where the separation was by stores (modules), not by separate files within a store.

To explore this further then, are you suggesting that we might want to consider applying some more granular namespacing here and split these up into more stores? Of course, that could introduce some complications with deprecating the old actions etc but might be preferable over the file splitting. So we could end up with something like core/editor/posts, core/editor (for general editor actions), and core/editor/blocks for block actions?

export * from './post-generators';
89 changes: 82 additions & 7 deletions packages/editor/src/store/actions/post-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,42 @@ import { BEGIN, COMMIT, REVERT } from 'redux-optimist';
import { POST_UPDATE_TRANSACTION_ID } from '../constants';

/**
* Optimistic action for requesting post update start.
* Returns an action object used in signalling that the latest version of the
* post has been received, either by initialization or save.
*
* @param {Object} post Post object.
*
* @return {Object} Action object.
*/
export function resetPost( post ) {
return {
type: 'RESET_POST',
post,
};
}

/**
* Returns an action object used in signalling that the latest autosave of the
* post has been received, by initialization or autosave.
*
* @param {Object} post Autosave post object.
*
* @return {Object} Action object.
*/
export function resetAutosave( post ) {
return {
type: 'RESET_AUTOSAVE',
post,
};
}

/**
* Optimistic action for dispatching that a post update request has started.
*
* @param {Object} options
* @return {Object} An action object
*/
export function requestPostUpdateStart( options = {} ) {
export function __experimentalRequestPostUpdateStart( options = {} ) {
return {
type: 'REQUEST_POST_UPDATE_START',
optimist: { type: BEGIN, id: POST_UPDATE_TRANSACTION_ID },
Expand All @@ -23,7 +53,8 @@ export function requestPostUpdateStart( options = {} ) {
}

/**
* Optimistic action for requesting post update success.
* Optimistic action for indicating that the request post update has completed
* successfully.
*
* @param {Object} previousPost The previous post prior to update.
* @param {Object} post The new post after update
Expand All @@ -33,7 +64,7 @@ export function requestPostUpdateStart( options = {} ) {
* @param {Object} postType The post type object.
* @return {Object} Action object.
*/
export function requestPostUpdateSuccess( {
export function __experimentalRequestPostUpdateSuccess( {
previousPost,
post,
isRevision,
Expand All @@ -58,7 +89,8 @@ export function requestPostUpdateSuccess( {
}

/**
* Optimistic action for requesting post update failure.
* Optimistic action for indicating that the request post update has completed
* with a failure.
*
* @param {Object} post The post that failed updating.
* @param {Object} edits The fields that were being updated.
Expand All @@ -67,7 +99,7 @@ export function requestPostUpdateSuccess( {
* dispatch.
* @return {Object} An action object
*/
export function requestPostUpdateFailure( {
export function __experimentalRequestPostUpdateFailure( {
post,
edits,
error,
Expand Down Expand Up @@ -98,16 +130,59 @@ export function updatePost( edits ) {
};
}

/**
* Returns an action object used in signalling that attributes of the post have
* been edited.
*
* @param {Object} edits Post attributes to edit.
*
* @return {Object} Action object.
*/
export function editPost( edits ) {
return {
type: 'EDIT_POST',
edits,
};
}

/**
* Returns action object produced by the updatePost creator augmented by
* an optimist option that signals optimistically applying updates.
*
* @param {Object} edits Updated post fields.
* @return {Object} Action object.
*/
export function optimisticUpdatePost( edits ) {
export function __experimentalOptimisticUpdatePost( edits ) {
return {
...updatePost( edits ),
optimist: { id: POST_UPDATE_TRANSACTION_ID },
};
}

/**
* Returns an action object used to signal that post saving is locked.
*
* @param {string} lockName The lock name.
*
* @return {Object} Action object
*/
export function lockPostSaving( lockName ) {
return {
type: 'LOCK_POST_SAVING',
lockName,
};
}

/**
* Returns an action object used to signal that post saving is unlocked.
*
* @param {string} lockName The lock name.
*
* @return {Object} Action object
*/
export function unlockPostSaving( lockName ) {
return {
type: 'UNLOCK_POST_SAVING',
lockName,
};
}
33 changes: 26 additions & 7 deletions packages/editor/src/store/actions/post-generators.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export function* savePost( options = {} ) {

yield dispatch(
MODULE_KEY,
'requestPostUpdateStart',
'__experimentalRequestPostUpdateStart',
options,
);

Expand All @@ -99,7 +99,7 @@ export function* savePost( options = {} ) {
// if the autosave is applied as a revision.
yield dispatch(
MODULE_KEY,
'optimisticUpdatePost',
'__experimentalOptimisticUpdatePost',
toSend
);

Expand Down Expand Up @@ -142,7 +142,7 @@ export function* savePost( options = {} ) {

yield dispatch(
MODULE_KEY,
'requestPostUpdateSuccess',
'__experimentalRequestPostUpdateSuccess',
{
previousPost: post,
post: newPost,
Expand All @@ -159,6 +159,7 @@ export function* savePost( options = {} ) {
previousPost: post,
post: newPost,
postType,
options,
} );
if ( notifySuccessArgs.length > 0 ) {
yield dispatch(
Expand All @@ -170,7 +171,7 @@ export function* savePost( options = {} ) {
} catch ( error ) {
yield dispatch(
MODULE_KEY,
'requestPostUpdateFailure',
'__experimentalRequestPostUpdateFailure',
{ post, edits, error, options }
);
const notifyFailArgs = getNotificationArgumentsForSaveFail( {
Expand All @@ -188,6 +189,15 @@ export function* savePost( options = {} ) {
}
}

/**
* Action generator used in signalling that the post should autosave.
*
* @param {Object?} options Extra flags to identify the autosave.
*/
export function* autosave( options ) {
yield* actions.savePost( { isAutosave: true, ...options } );
}

/**
* Action generator for trashing the current post in the editor.
*/
Expand Down Expand Up @@ -218,8 +228,8 @@ export function* trashPost() {
}
);

// TODO: This should be an updatePost action (updating subsets of post properties),
// But right now editPost is tied with change detection.
// TODO: This should be an updatePost action (updating subsets of post
// properties), but right now editPost is tied with change detection.
yield dispatch(
MODULE_KEY,
'resetPost',
Expand All @@ -229,7 +239,7 @@ export function* trashPost() {
yield dispatch(
'core/notices',
'createErrorNotice',
getNotificationArgumentsForTrashFail( { error } ),
...getNotificationArgumentsForTrashFail( { error } ),
);
}
}
Expand Down Expand Up @@ -264,3 +274,12 @@ export function* refreshPost() {
newPost
);
}

const actions = {
savePost,
autosave,
trashPost,
refreshPost,
};

export default actions;
Loading