Remove all wp.api Usage from the editor package #8088

youknowriad · 2018-07-20T15:05:56Z

This PR tries to remove all wp.api usage from the "editor" package.

Notes

Splits effects file for sanity and also to make it easier to use async/await
Adds a resolveSelector utility function that waits for the resolution of the selector before returning a promise with its value.
Replaces wp.api.getPostTypeRoute with select( 'core' ).getPostType( getCurrentPostType( state ) ).rest_base

Testing instructions

Ensure shared blocks work
Ensure taxonomy selectors work
Ensure saving posts work

youknowriad · 2018-07-20T15:06:47Z

editor/store/effects/posts.js

@@ -0,0 +1,320 @@
+/**


For more sanity and also because it's easier to use async/await this way, I extracted some effects handler into their own files.

youknowriad · 2018-07-20T15:07:24Z

editor/store/effects.js

+	REQUEST_POST_UPDATE_SUCCESS: requestPostUpdateSuccess,
+	REQUEST_POST_UPDATE_FAILURE: requestPostUpdateFailure,
+	TRASH_POST: ( action, store ) => {
+		trashPost( action, store );


I can't assign it directly because there's a warning if we return a promise from effect handlers

youknowriad · 2018-07-20T15:08:43Z

editor/store/effects/utils.js

+		const getResult = () => select( namespace )[ selectorName ].apply( null, args );
+
+		const unsubscribe = subscribe( () => {
+			if ( hasFinished() ) {


This is where it's not working at the moment, I'm not certain why but hasFinished is never true.

Try to edit a post and click "save draft", nothing happens because it's stuck here.

cc @aduth

Fixed by aduth/equivalent-key-map#1

gziolo

I would like to better distill what has exactly changed with the effects file refactoring. I will wait until you are online before I continue. In general, I like the direction where it is heading. It will also help us to address #7397 with the removel of withAPIData usage.

gziolo · 2018-07-23T06:58:49Z

editor/components/post-taxonomies/flat-term-selector.js

+			hasCreateAction: taxonomy ? get( getCurrentPost(), [ '_links', 'wp:action-create-' + taxonomy.rest_base ], false ) : false,
+			hasAssignAction: taxonomy ? get( getCurrentPost(), [ '_links', 'wp:action-assign-' + taxonomy.rest_base ], false ) : false,
+			terms: taxonomy ? select( 'core/editor' ).getEditedPostAttribute( taxonomy.rest_base ) : [],
+			taxonomy,


Should we pass down restBase instead of full taxonomy object? I think only this one field is used in the component. It would also align better with what we had before.

Actually, this made me think, what will happen when taxonomy is falsy? We don't check against it inside the component.

We check against hasAssignAction which is false if the taxonomy is falsy.

And the taxonomy object is used for labels as well.

Do we expect to refactor this component so that it's not calling apiFetch at all? This was a hope of mine. Wondering if it would make things easier to do this conversion first, before this one.

Yes, I think we should do it, the thing is, it's not ready at the moment because we need paginated/query data in core-data. Hopefully we can get back to that once we publish those remaining packages.

gziolo · 2018-07-23T07:06:19Z

editor/components/post-taxonomies/hierarchical-term-selector.js

+			hasCreateAction: taxonomy ? get( getCurrentPost(), [ '_links', 'wp:action-create-' + taxonomy.rest_base ], false ) : false,
+			hasAssignAction: taxonomy ? get( getCurrentPost(), [ '_links', 'wp:action-assign-' + taxonomy.rest_base ], false ) : false,
+			terms: taxonomy ? select( 'core/editor' ).getEditedPostAttribute( taxonomy.rest_base ) : [],
+			taxonomy,


I echo my previous concerns regarding taxonomy and rest_base.

I think I added a check for this specific component

youknowriad · 2018-07-23T13:22:28Z

editor/store/effects/utils.js

+ *
+ * @return {Promise} Selector result.
+ */
+export function resolveSelector( namespace, selectorName, ...args ) {


This utility or something equivalent will be bundled in the data module once we land the async actions work.

gziolo

One nitpick so far, I will continue tomorrow. taxonomy changes look good as a temporary solution. I double checked your comments 👍

gziolo · 2018-07-24T13:36:28Z

package.json

 		"dom-scroll-into-view": "1.2.1",
 		"element-closest": "2.0.2",
-		"equivalent-key-map": "0.2.0",
+		"equivalent-key-map": "0.2.1",


We can remove it from here, it is used only in data package.

For some reason, I didn't understand, removing it break the unit tests (but only on travis)

Notice I removed it and than added it back in the commits

I think there went something wrong with package-lock.json. We can clean it up when all packages are there, no worries 👍

Copons · 2018-07-25T15:18:44Z

Not sure if relevant, but I've found a wp.api here:

gutenberg/editor/components/provider/index.js

Lines 71 to 85 in a5f4c9d

    
           // APIProvider 
        
           // 
        
           //  - context.getAPISchema 
        
           //  - context.getAPIPostTypeRestBaseMapping 
        
           //  - context.getAPITaxonomyRestBaseMapping 
        
           [ 
        
           	APIProvider, 
        
           	{ 
        
           		...wpApiSettings, 
        
           		...pick( wp.api, [ 
        
           			'postTypeRestBaseMapping', 
        
           			'taxonomyRestBaseMapping', 
        
           		] ), 
        
           	}, 
        
           ],

Copons · 2018-07-25T15:26:23Z

Tested for a bit and couldn't break anything! ✨

Ensure shared blocks work
Ensure taxonomy selectors work
Ensure saving posts work

gwwar · 2018-07-25T18:27:43Z

Re:

gutenberg/editor/components/provider/index.js

Lines 71 to 85 in a5f4c9d

    
           // APIProvider 
        
           // 
        
           //  - context.getAPISchema 
        
           //  - context.getAPIPostTypeRestBaseMapping 
        
           //  - context.getAPITaxonomyRestBaseMapping 
        
           [ 
        
           	APIProvider, 
        
           	{ 
        
           		...wpApiSettings, 
        
           		...pick( wp.api, [ 
        
           			'postTypeRestBaseMapping', 
        
           			'taxonomyRestBaseMapping', 
        
           		] ), 
        
           	}, 
        
           ],

It looks like wpApiSettings, wp.api.postTypeRestBaseMapping and wp.api.taxonomyRestBaseMapping are all globals, and we'll run into trouble trying to resolve these in the published package.

Could we provide defaults, or pass through props to this component, with a fallback to globals if they exist?

youknowriad · 2018-07-25T18:31:37Z

Good catch @gwwar and @Copons I feel like we can get rid of them entirely as withApiData is meant to be deprecated anyway. I'll see tomorrow if I can refactor the components using them.

gziolo · 2018-07-26T07:20:44Z

editor/store/effects/utils.js

+
+		// We need to trigger the selector (to trigger the resolver)
+		const result = getResult();
+		if ( hasFinished() ) {


Can unsubscribe be declared after this if statement so we wouldn't have to subscribe it if is already finished?

Unit test passes, so it should be good :)

gziolo · 2018-07-26T07:33:38Z

editor/store/effects/test/utils.js

+		} );
+
+		// Trigger resolution
+		const value = await resolveSelector( 'resolveStore', 'selectAll', 'check' );


Should it be resolveStore2?

gziolo · 2018-07-26T07:39:49Z

editor/store/effects/test/utils.js

+
+describe( 'resolveSelector', () => {
+	it( 'Should wait for selector resolution', async () => {
+		registerStore( 'resolveStore', {


Would it make to extract the store definitions as follows:

const resolveStore = { reducer: ( state = 'no', action ) => { if ( action.type === 'resolve' ) { return 'yes'; } return state; }, selectors: { selectAll: ( state, key ) => ( key === 'check' ) ? state : 'no-key', }, resolvers: { selectAll: () => { return new Promise( ( resolve ) => { process.nextTick( () => resolve( { type: 'resolve' } ) ); } ); }, }, };

and share it between those 2 tests?

gziolo · 2018-07-26T07:39:55Z

editor/store/effects/test/utils.js

+				if ( action.type === 'resolve' ) {
+					return 'yes';
+				}
+				if ( action.type === 'reset' ) {


This type is never used. Removing it doesn't break any test.

gziolo

I finished my review, how do you feel about addressing smaller things in this PR and opening a follow up for what @Copons discovered? This PR is already big and I bet we will have to move Provider code outside of editor to make it backward compatible. We need to ensure withAPIData works for some time before we fully deprecate it.

gziolo · 2018-07-26T08:28:03Z

editor/store/effects/posts.js

+	dispatch( removeNotice( TRASH_POST_NOTICE_ID ) );
+	try {
+		await apiFetch( { path: `/wp/v2/${ postType.rest_base }/${ postId }`, method: 'DELETE' } );
+	} catch ( error ) {


I see this code missing after apiFetch when comparing with master:

const post = getCurrentPost( getState() ); // TODO: This should be an updatePost action (updating subsets of post properties), // But right now editPost is tied with change detection. dispatch( resetPost( { ...post, status: 'trash' } ) );

Is that expected change?

No it's not, I wonder how this happened :P

gziolo · 2018-07-26T08:35:43Z

editor/store/effects/shared-blocks.js

+	// TODO: these are potentially undefined, this fix is in place
+	// until there is a filter to not use shared blocks if undefined
+	const postType = await resolveSelector( 'core', 'getPostType', 'wp_block' );
+	if ( ! postType ) {


I'm not sure how to check the return value for this selector. We should ensure it is not an empty object because this check will fail.

Just in case it's necessary, there are 2 more places where we have the same check.

I don't think it can be an empty object, if the state is not there for some reason, it will be undefined and it will be the right post type if not. I wonder if we can't just remove the check entirely actually :P

gziolo · 2018-07-26T10:02:01Z

It looks like wpApiSettings, wp.api.postTypeRestBaseMapping and wp.api.taxonomyRestBaseMapping are all globals, and we'll run into trouble trying to resolve these in the published package.

Could we provide defaults, or pass through props to this component, with a fallback to globals if they exist?

It looks like @youknowriad found a really nice solution for that one :)

youknowriad · 2018-07-26T10:04:51Z

I removed the remaining dependencies to wp.api. I didn't deprecate withAPIData yet but I think we're ready for it. This PR should be ready to go.

gziolo

I tested locally a similar set of functionalities as @Copons did. Everything works as expected. Travis is failing with one of the e2e tests, but it's a known issue and we have open PR to fix it. I restart build. Feel free to merge as soon as it turns green.

Great job with this refactor 🥇

gwwar · 2018-07-26T16:37:02Z

editor/components/provider/index.js

 				APIProvider,
-				{
-					...wpApiSettings,
-					...pick( wp.api, [


aduth · 2018-08-06T19:17:10Z

editor/store/effects/posts.js

+const TRASH_POST_NOTICE_ID = 'TRASH_POST_NOTICE_ID';
+
+/**
+ * Request Post Update Effect handler


These JSDoc descriptions really go to reinforce my worry that we don't really have a good understanding of what's intended to take place in these functions.

Remove all wp.api Usage from the editor package

91184f5

youknowriad commented Jul 20, 2018

View reviewed changes

gziolo mentioned this pull request Jul 23, 2018

Core blocks: Stop using withAPIData for Latest Posts block in code and docs #7397

Closed

gziolo reviewed Jul 23, 2018

View reviewed changes

gziolo added npm Packages Related to npm packages [Type] Code Quality Issues or PRs that relate to code quality labels Jul 23, 2018

gziolo added this to the 3.4 milestone Jul 23, 2018

gziolo assigned youknowriad Jul 23, 2018

youknowriad added 3 commits July 23, 2018 12:13

Adding unit tests

7880b57

Update equivalent-key-map to fix resolution flags issue

0ef3c00

Fix shared block creation

9e976ee

youknowriad commented Jul 23, 2018

View reviewed changes

youknowriad force-pushed the remove/ap-api-from-editor branch from 4d993d5 to 8fce9d6 Compare July 23, 2018 13:48

Fix tests

a5f4c9d

youknowriad force-pushed the remove/ap-api-from-editor branch from 8fce9d6 to a5f4c9d Compare July 23, 2018 14:16

youknowriad changed the title ~~WIP: Remove all wp.api Usage from the editor package~~ Remove all wp.api Usage from the editor package Jul 24, 2018

youknowriad mentioned this pull request Jul 24, 2018

Packages: Extract the editor package #8081

Merged

3 tasks

gziolo reviewed Jul 24, 2018

View reviewed changes

gziolo requested review from gwwar, jorgefilipecosta and tofumatt July 25, 2018 14:03

gwwar requested a review from vindl July 25, 2018 14:35

gziolo reviewed Jul 26, 2018

View reviewed changes

youknowriad added 2 commits July 26, 2018 10:55

Remove remaining dependencies to wp.api

4d1d22d

Tweak the resolveSelector unit tests

933d366

Restore accidentally removed code

cd8832c

Fix unit tests

20916c5

gziolo approved these changes Jul 26, 2018

View reviewed changes

youknowriad merged commit 740de8d into master Jul 26, 2018

youknowriad deleted the remove/ap-api-from-editor branch July 26, 2018 11:03

gwwar reviewed Jul 26, 2018

View reviewed changes

editor/components/provider/index.js

APIProvider,

{

...wpApiSettings,

...pick( wp.api, [

Copy link

Contributor

gwwar Jul 26, 2018

Choose a reason for hiding this comment

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

✨

aduth reviewed Aug 6, 2018

View reviewed changes

maxxer mentioned this pull request Sep 3, 2018

Error Opening Posts with Gutenberg 3.7.0 Plugin Yoast/wordpress-seo#10881

Closed

8 tasks

Remove all wp.api Usage from the editor package #8088

Remove all wp.api Usage from the editor package #8088

Uh oh!

Conversation

youknowriad commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copons commented Jul 25, 2018

Uh oh!

Copons commented Jul 25, 2018

Uh oh!

gwwar commented Jul 25, 2018

Uh oh!

youknowriad commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Jul 26, 2018

Uh oh!

youknowriad commented Jul 26, 2018

Uh oh!

youknowriad commented Jul 20, 2018 •

edited

Loading