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.

Sign up for GitHub

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

Jump to bottom

Refactor setupEditor effects to actions #14513

Merged
merged 7 commits into from
Mar 22, 2019
Merged

Refactor setupEditor effects to actions #14513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ea41379
remove unnecessary test file for effects
nerrad Mar 19, 2019
0f590bb
refactor setupEditor effects to action-generator
nerrad Mar 19, 2019
f320abc
add changelog entry
nerrad Mar 19, 2019
8ca0b55
update docs
nerrad Mar 19, 2019
451de2a
remove no longer needed redux-multi dependency/usage
nerrad Mar 21, 2019
ce1e4fa
update changelog about redux-multi removal
nerrad Mar 21, 2019
46a53e0
yield actions directly instead of controls
nerrad Mar 22, 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
2 changes: 1 addition & 1 deletion docs/designers-developers/developers/data/data-core-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ The editor settings object.

### setupEditor

Returns an action object used in signalling that editor has initialized with
Returns an action generator used in signalling that editor has initialized with
the specified post object and editor settings.

*Parameters*
Expand Down
1 change: 0 additions & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions packages/editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 9.1.1 (Unreleased)

### Internal

- Refactor setupEditor effects to action-generator using controls ([#14513](https://github.com/WordPress/gutenberg/pull/14513))
- Remove redux-multi dependency (no longer needed/used with above refactor)

## 9.1.0 (2019-03-06)

### New Features
Expand Down
1 change: 0 additions & 1 deletion packages/editor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
"lodash": "^4.17.11",
"memize": "^1.0.5",
"react-autosize-textarea": "^3.0.2",
"redux-multi": "^0.1.12",
"redux-optimist": "^1.0.0",
"refx": "^3.0.0",
"rememo": "^3.0.0",
Expand Down
39 changes: 33 additions & 6 deletions packages/editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { castArray, pick } from 'lodash';
import { castArray, pick, has } from 'lodash';
import { BEGIN, COMMIT, REVERT } from 'redux-optimist';

/**
Expand All @@ -26,22 +26,49 @@ import {
} from './utils/notice-builder';

/**
* Returns an action object used in signalling that editor has initialized with
* WordPress dependencies
*/
import {
parse,
synchronizeBlocksWithTemplate,
} from '@wordpress/blocks';

/**
* Returns an action generator used in signalling that editor has initialized with
* the specified post object and editor settings.
*
* @param {Object} post Post object.
* @param {Object} edits Initial edited attributes object.
* @param {Array?} template Block Template.
*
* @return {Object} Action object.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd wondered if JSDoc had some option to document generator function yields. It appears @yields is a thing, a suitable replacement for @return.

http://usejsdoc.org/tags-yields.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I had thought about using @yields but the difficulty here is that most if not all of these actions do not have a single yield. So it's a bit tricky knowing what exactly to document.

I've sort of loosely followed the pattern of only include a @return tag on generators when there are return values in the generator, otherwise having nothing.

Copy link
Member

Choose a reason for hiding this comment

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

Added @yields as a feature request for the docgen default formatter: #14583

*/
export function setupEditor( post, edits, template ) {
return {
export function* setupEditor( post, edits, template ) {
yield {
type: 'SETUP_EDITOR',
post,
edits,
template,
};

// In order to ensure maximum of a single parse during setup, edits are
// included as part of editor setup action. Assume edited content as
// canonical if provided, falling back to post.
let content;
if ( has( edits, [ 'content' ] ) ) {
content = edits.content;
} else {
content = post.content.raw;
}

let blocks = parse( content );

// Apply a template for new posts only, if exists.
const isNewPost = post.status === 'auto-draft';
if ( isNewPost && template ) {
blocks = synchronizeBlocksWithTemplate( blocks, template );
}

yield resetEditorBlocks( blocks );
yield setupEditorState( post );
}

/**
Expand Down
43 changes: 0 additions & 43 deletions packages/editor/src/store/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,6 @@
/**
* External dependencies
*/
import { has } from 'lodash';

/**
* WordPress dependencies
*/
import {
parse,
synchronizeBlocksWithTemplate,
} from '@wordpress/blocks';

/**
* Internal dependencies
*/
import {
setupEditorState,
resetEditorBlocks,
} from './actions';
import {
fetchReusableBlocks,
saveReusableBlocks,
Expand All @@ -28,32 +11,6 @@ import {
} from './effects/reusable-blocks';

export default {
SETUP_EDITOR( action ) {
const { post, edits, template } = action;

// In order to ensure maximum of a single parse during setup, edits are
// included as part of editor setup action. Assume edited content as
// canonical if provided, falling back to post.
let content;
if ( has( edits, [ 'content' ] ) ) {
content = edits.content;
} else {
content = post.content.raw;
}

let blocks = parse( content );

// Apply a template for new posts only, if exists.
const isNewPost = post.status === 'auto-draft';
if ( isNewPost && template ) {
blocks = synchronizeBlocksWithTemplate( blocks, template );
}

return [
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that this was the only occurrence of an array-action for which we apply the redux-multi middleware (i.e. upon confirmation, it may be warranted to remove as part of these changes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't follow: The refactor changed this from an array action to dispatch actions. So the work in this pull already removes the reliance on the redux-multi middleware for setupEditor? Or do you mean that we can remove the redux-multi dependency because of this (which makes sense).

Copy link
Member

Choose a reason for hiding this comment

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

Or do you mean that we can remove the redux-multi dependency because of this (which makes sense).

Yes, this.

Copy link
Contributor Author

@nerrad nerrad Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't look like anything else was using this in the editor package so went ahead and removed it. I think I did things correctly (just removed from package.json) but with this being a mono-repo I'm not sure :)

removed in 52af9b3

Copy link
Member

Choose a reason for hiding this comment

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

I think I did things correctly (just removed from package.json) but with this being a mono-repo I'm not sure :)

The one-line removal from package-lock.json seems correct since it's still a dependency in block-editor (though perhaps it shouldn't be; a point outside the scope of this pull request).

In any case, Travis would have caught it anyways if it was an issue, and it appears to be just fine.

resetEditorBlocks( blocks ),
setupEditorState( post ),
];
},
FETCH_REUSABLE_BLOCKS: ( action, store ) => {
fetchReusableBlocks( action, store );
},
Expand Down
12 changes: 2 additions & 10 deletions packages/editor/src/store/middlewares.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
* External dependencies
*/
import refx from 'refx';
import multi from 'redux-multi';
import { flowRight } from 'lodash';

/**
* Internal dependencies
Expand All @@ -18,25 +16,19 @@ import effects from './effects';
* @return {Object} Update Store Object.
*/
function applyMiddlewares( store ) {
const middlewares = [
refx( effects ),
multi,
];

let enhancedDispatch = () => {
throw new Error(
'Dispatching while constructing your middleware is not allowed. ' +
'Other middleware would not be applied to this dispatch.'
);
};
let chain = [];

const middlewareAPI = {
getState: store.getState,
dispatch: ( ...args ) => enhancedDispatch( ...args ),
};
chain = middlewares.map( ( middleware ) => middleware( middlewareAPI ) );
enhancedDispatch = flowRight( ...chain )( store.dispatch );

enhancedDispatch = refx( effects )( middlewareAPI )( store.dispatch );

store.dispatch = enhancedDispatch;
return store;
Expand Down
33 changes: 26 additions & 7 deletions packages/editor/src/store/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,16 +625,35 @@ describe( 'Post generator actions', () => {
} );
} );

describe( 'actions', () => {
describe( 'setupEditor', () => {
it( 'should return the SETUP_EDITOR action', () => {
const post = {};
const result = actions.setupEditor( post );
expect( result ).toEqual( {
type: 'SETUP_EDITOR',
describe( 'Editor actions', () => {
describe( 'setupEditor()', () => {
let fulfillment;
const reset = ( post, edits, template ) => fulfillment = actions
.setupEditor(
post,
edits,
template,
);
it( 'should yield the SETUP_EDITOR action', () => {
reset( { content: { raw: '' }, status: 'publish' } );
const { value } = fulfillment.next();
expect( value ).toEqual( {
type: 'SETUP_EDITOR',
post: { content: { raw: '' }, status: 'publish' },
} );
} );
it( 'should yield action object for resetEditorBlocks', () => {
const { value } = fulfillment.next();
expect( value ).toEqual( actions.resetEditorBlocks( [] ) );
} );
it( 'should yield action object for setupEditorState', () => {
const { value } = fulfillment.next();
expect( value ).toEqual(
actions.setupEditorState(
{ content: { raw: '' }, status: 'publish' }
)
);
} );
} );

describe( 'resetPost', () => {
Expand Down
88 changes: 0 additions & 88 deletions packages/editor/src/store/test/effects.js
View file
Open in desktop

This file was deleted.