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

Fix Nav Block bug as Contributor user via fix to *loadPostTypeEntities #18669

Merged
merged 17 commits into from
Apr 1, 2020
Merged

Fix Nav Block bug as Contributor user via fix to *loadPostTypeEntities #18669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
880d127
Utilise view context to ensure access to all post types.
getdave Nov 21, 2019
24a1fec
Allow context to be overidden in query args
getdave Nov 21, 2019
b8465d7
Set Nav to use “view” REST API context
getdave Nov 21, 2019
46b0980
Revert "Utilise view context to ensure access to all post types."
getdave Nov 21, 2019
83b5c56
Revert "Allow context to be overidden in query args"
getdave Nov 21, 2019
07a0821
Revert "Set Nav to use “view” REST API context"
getdave Nov 21, 2019
013d759
Initial naive refactor to use apiFetch directly.
getdave Nov 21, 2019
d9287c5
Improves handling of API request and adds comments
getdave Nov 21, 2019
55bf64c
Fix errors from conflict solving.
tellthemachines Mar 24, 2020
11a4ff8
lint
tellthemachines Mar 25, 2020
8bbe13a
Extracted fetch hook
tellthemachines Mar 25, 2020
90f92fa
Added jsdoc
tellthemachines Mar 25, 2020
dab1a15
Change API pattern.
tellthemachines Mar 31, 2020
0a5d67f
Clean up booleans and conditions
tellthemachines Mar 31, 2020
5b89402
Move hook to api-fetch package
tellthemachines Mar 31, 2020
585a6db
Attach hook to default export
tellthemachines Mar 31, 2020
641ba9e
Reset state before fetching, don't guess data type
tellthemachines Mar 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions 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.

1 change: 1 addition & 0 deletions packages/api-fetch/package.json
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
"react-native": "src/index",
"dependencies": {
"@babel/runtime": "^7.8.3",
"@wordpress/element": "file:../element",
"@wordpress/i18n": "file:../i18n",
"@wordpress/url": "file:../url"
},
Expand Down
39 changes: 39 additions & 0 deletions packages/api-fetch/src/index.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useEffect, useState } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -156,6 +157,42 @@ function apiFetch( options ) {
} );
}

/**
* Function that fetches data using apiFetch, and updates the status.
*
* @param {string} path Query path.
*/
function useApiFetch( path ) {
Copy link
Member

Choose a reason for hiding this comment

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

By naming standards, this should be useAPIFetch.

See: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/#abbreviations-and-acronyms

// Indicate the fetching status
const [ isLoading, setIsLoading ] = useState( true );
const [ data, setData ] = useState( null );
const [ error, setError ] = useState( null );

useEffect( () => {
setIsLoading( true );
setData( null );
setError( null );

apiFetch( { path } )
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be wrong, but I'm wondering if the state should be reset prior to calling apiFetch by calling setIsLoading, setData and setError again with the initial values, as I think if the path changes, useState won't override any existing values being stored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh good point, it probably won't because only the effect will re-run.

.then( ( fetchedData ) => {
setData( fetchedData );
// We've stopped fetching
setIsLoading( false );
} )
.catch( ( err ) => {
setError( err );
// We've stopped fetching
setIsLoading( false );
} );
Comment on lines +177 to +186
Copy link
Member

Choose a reason for hiding this comment

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

There's no guarantee that the component is still mounted at the point we're calling setState here. If it was removed in the time between the request starting and its completion, the state setters will fail, and React will log a warning to the console.

See also: https://reactjs.org/docs/hooks-reference.html#cleaning-up-an-effect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I also reinforce that this anti pattern is super easy to introduce. I wonder whether we could make the hook handle this edge cases automatically by:

  1. Wrapping the fetch Promise to allow to be cancelled.
  2. Automatically calling the .cancel() on unmount.
  3. Checking for isCancelled state in .then / .catch handlers and exiting early ( to avoid state updates).

For an example see Automattic/jetpack#15228

Copy link
Contributor

@nerrad nerrad Apr 2, 2020 •
edited

Choose a reason for hiding this comment

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

This point makes me think of the behaviour of resolvers in wp.data which often include making fetch requests. This same issue can surface there via usage of useSelect with selectors that resolve (and I think already is). We probably should be looking into that too.

Copy link
Member

@noisysocks noisysocks Apr 3, 2020 •
edited

Choose a reason for hiding this comment

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

Maybe we could promote good behaviour1 here by having apiFetch return a function that cancels the request.

useEffect( () => {
	setIsLoading( true );
	setData( null );
	setError( null );

	const cancelFetch = apiFetch( { path } )
		.then( ( fetchedData ) => {
			setData( fetchedData );
			// We've stopped fetching
			setIsLoading( false );
		} )
		.catch( ( err ) => {
			setError( err );
			// We've stopped fetching
			setIsLoading( false );
		} );

	return cancelFetch;
}, [ path ] );

1: If it's easy to do the right thing, it's more likely that one will do the right thing 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I'm motivated toward anything which keeps apiFetch cumbersome to use in a component, if it means that the preferred core data entities are the more obvious solution by virtue of their ergonomics advantage. Which isn't to say there's not issues there as well (#18669 (comment)), but I think we're in a better position to solve those internal to those implementations, even if it's something like this.

}, [ path ] );

return {
isLoading,
data,
error,
};
}

apiFetch.use = registerMiddleware;
apiFetch.setFetchHandler = setFetchHandler;

Expand All @@ -165,4 +202,6 @@ apiFetch.createRootURLMiddleware = createRootURLMiddleware;
apiFetch.fetchAllMiddleware = fetchAllMiddleware;
apiFetch.mediaUploadMiddleware = mediaUploadMiddleware;

apiFetch.useApiFetch = useApiFetch;

export default apiFetch;
59 changes: 27 additions & 32 deletions packages/block-library/src/navigation/edit.js
View file
Open in desktop
Expand Up @@ -32,7 +32,8 @@ import {
import { compose } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import { menu } from '@wordpress/icons';

import { addQueryArgs } from '@wordpress/url';
import { useApiFetch } from '@wordpress/api-fetch';
/**
* Internal dependencies
*/
Expand All @@ -46,9 +47,6 @@ function Navigation( {
clientId,
fontSize,
hasExistingNavItems,
hasResolvedPages,
isRequestingPages,
pages,
setAttributes,
setFontSize,
updateNavItemBlocks,
Expand All @@ -57,10 +55,10 @@ function Navigation( {
//
// HOOKS
//
/* eslint-disable @wordpress/no-unused-vars-before-return */
const ref = useRef();
const { selectBlock } = useDispatch( 'core/block-editor' );

/* eslint-disable @wordpress/no-unused-vars-before-return */
Copy link
Member

Choose a reason for hiding this comment

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

Do we find ourselves having to disable this often in components? If it's not a useful rule then we should just remove it...

(Not relevant to this PR!)

Copy link
Member

Choose a reason for hiding this comment

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

The value of the rule is explained here:

https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md

Based on specific instances, we can choose to improve it as well. For example, it should probably be exempting hooks as a special circumstances, due to constraints imposed on us externally via Rules of Hooks.

Copy link
Member

Choose a reason for hiding this comment

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

The value of the rule is explained here:

I should have also mentioned there was previously quite a few offending cases that were cleaned up as part of the introduction of the rule, in case that's a more convincing method for interpreting the value.

See: #12827

Copy link
Member

Choose a reason for hiding this comment

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

It might not have been necessary to disable the rule here anyways? It was removed in #20884. If I recall correctly, the rule makes exceptions for object destructuring.

To the general problem:

we can choose to improve it as well. For example, it should probably be exempting hooks as a special circumstances, due to constraints imposed on us externally via Rules of Hooks.

This already exists as of #16737:

gutenberg/packages/eslint-plugin/configs/react.js

Lines 10 to 15 in 1f00c74

'@wordpress/no-unused-vars-before-return': [
'error',
{
excludePattern: '^use',
},
],

The problem here is that the hook is named as experimental, __experimentalUseColors. There are two ways we could accommodate for this:

  1. We can adopt a convention to reference the slot by its non-experimental name in the implementation of the component, renaming via the import (example)
  2. We can expand the pattern above to include __experimental and __unstable naming conventions

const {
TextColor,
BackgroundColor,
Expand All @@ -86,15 +84,37 @@ function Navigation( {
},
[ fontSize.size ]
);

/* eslint-enable @wordpress/no-unused-vars-before-return */

const { navigatorToolbarButton, navigatorModal } = useBlockNavigator(
clientId
);

const baseUrl = '/wp/v2/pages';

// "view" is required to ensure Pages are returned by REST API
// for users with lower capabilities such as "Contributor" otherwise
// Pages are not returned in the request if "edit" context is set
const context = 'view';

const filterDefaultPages = {
parent: 0,
order: 'asc',
orderby: 'id',
context,
};

const queryPath = addQueryArgs( baseUrl, filterDefaultPages );

const { isLoading: isRequestingPages, data: pages } = useApiFetch(
queryPath
);

const hasPages = !! pages;

// Builds navigation links from default Pages.
const defaultPagesNavigationItems = useMemo( () => {
if ( ! pages ) {
if ( ! hasPages ) {
return null;
}

Expand Down Expand Up @@ -134,8 +154,6 @@ function Navigation( {
selectBlock( clientId );
}

const hasPages = hasResolvedPages && pages && pages.length;

const blockClassNames = classnames( className, {
[ `items-justified-${ attributes.itemsJustification }` ]: attributes.itemsJustification,
[ fontSize.class ]: fontSize.class,
Expand Down Expand Up @@ -290,31 +308,8 @@ export default compose( [
withSelect( ( select, { clientId } ) => {
const innerBlocks = select( 'core/block-editor' ).getBlocks( clientId );

const filterDefaultPages = {
parent: 0,
order: 'asc',
orderby: 'id',
};

const pagesSelect = [
'core',
'getEntityRecords',
[ 'postType', 'page', filterDefaultPages ],
];

return {
hasExistingNavItems: !! innerBlocks.length,
pages: select( 'core' ).getEntityRecords(
'postType',
'page',
filterDefaultPages
),
isRequestingPages: select( 'core/data' ).isResolving(
...pagesSelect
),
hasResolvedPages: select( 'core/data' ).hasFinishedResolution(
...pagesSelect
),
};
} ),
withDispatch( ( dispatch, { clientId } ) => {
Expand Down