-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Reusable blocks: prevent infinite recursion #28405
Changes from 10 commits
c228df2
1c5f5da
9392953
51deb88
f9c9dd6
2f93b2b
28d8c42
9c7d4af
90eb586
1ee9767
bee83e8
0537efe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { render } from '@testing-library/react'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { Fragment } from '@wordpress/element'; | ||
|
||
// Mimics a block's Edit component, such as ReusableBlockEdit, which is capable | ||
// of calling itself depending on its `ref` attribute. | ||
function Edit( { attributes: { ref } } ) { | ||
const [ hasAlreadyRendered, RecursionProvider ] = useNoRecursiveRenders( | ||
ref | ||
); | ||
|
||
if ( hasAlreadyRendered ) { | ||
return <div className="wp-block__reusable-block--halted">Halt</div>; | ||
} | ||
|
||
return ( | ||
<RecursionProvider> | ||
<div className="wp-block__reusable-block"> | ||
{ ref === 'SIMPLE' && <p>Done</p> } | ||
{ ref === 'SINGLY-RECURSIVE' && ( | ||
<Edit attributes={ { ref } } /> | ||
) } | ||
{ ref === 'MUTUALLY-RECURSIVE-1' && ( | ||
<Edit attributes={ { ref: 'MUTUALLY-RECURSIVE-2' } } /> | ||
) } | ||
{ ref === 'MUTUALLY-RECURSIVE-2' && ( | ||
<Edit attributes={ { ref: 'MUTUALLY-RECURSIVE-1' } } /> | ||
) } | ||
</div> | ||
</RecursionProvider> | ||
); | ||
} | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import useNoRecursiveRenders from '../use-no-recursive-renders'; | ||
|
||
describe( 'useNoRecursiveRenders', () => { | ||
it( 'allows a single block to render', () => { | ||
const { container } = render( | ||
<Edit attributes={ { ref: 'SIMPLE' } } /> | ||
); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block' ) | ||
).toHaveLength( 1 ); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block--halted' ) | ||
).toHaveLength( 0 ); | ||
} ); | ||
|
||
it( 'allows equal but sibling blocks to render', () => { | ||
const { container } = render( | ||
<Fragment> | ||
<Edit attributes={ { ref: 'SIMPLE' } } /> | ||
<Edit attributes={ { ref: 'SIMPLE' } } /> | ||
</Fragment> | ||
); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block' ) | ||
).toHaveLength( 2 ); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block--halted' ) | ||
).toHaveLength( 0 ); | ||
} ); | ||
|
||
it( 'prevents a block from rendering itself', () => { | ||
const { container } = render( | ||
<Fragment> | ||
<Edit attributes={ { ref: 'SINGLY-RECURSIVE' } } /> | ||
</Fragment> | ||
); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block' ) | ||
).toHaveLength( 1 ); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block--halted' ) | ||
).toHaveLength( 1 ); | ||
} ); | ||
|
||
it( 'prevents mutual recursion between two blocks', () => { | ||
const { container } = render( | ||
<Fragment> | ||
<Edit attributes={ { ref: 'MUTUALLY-RECURSIVE-1' } } /> | ||
</Fragment> | ||
); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block' ) | ||
).toHaveLength( 2 ); | ||
expect( | ||
container.querySelectorAll( '.wp-block__reusable-block--halted' ) | ||
).toHaveLength( 1 ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { | ||
createContext, | ||
useCallback, | ||
useContext, | ||
useMemo, | ||
} from '@wordpress/element'; | ||
|
||
const RenderedRefsContext = createContext( [] ); | ||
|
||
export default function useNoRecursiveRenders( ref ) { | ||
const previouslyRenderedRefs = useContext( RenderedRefsContext ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker for this PR. I wanted to share my thought on how to make it general enough to use with other blocks that require the same guard (Post Content or Template Part). It looks like it would be enough to rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, thanks for the note! |
||
const hasAlreadyRendered = previouslyRenderedRefs.includes( ref ); | ||
const newRenderedRefs = useMemo( () => [ ...previouslyRenderedRefs, ref ], [ | ||
ref, | ||
previouslyRenderedRefs, | ||
] ); | ||
const Provider = useCallback( | ||
( { children } ) => ( | ||
<RenderedRefsContext.Provider value={ newRenderedRefs }> | ||
{ children } | ||
</RenderedRefsContext.Provider> | ||
), | ||
[ newRenderedRefs ] | ||
); | ||
return [ hasAlreadyRendered, Provider ]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on leaving this here or returning
''
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go for returning it, as it can be overlooked that there is a problem, by having the warnings not visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if that is helpful for someone browsing the website. It's not that far to displaying an error when PHP can't connect with the database. You could display it maybe when
WP_DEBUG
is enabled but maybe with a ton of surrounding red colors 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with
WP_DEBUG_DISPLAY
, per:https://github.com/WordPress/wordpress-develop/blob/21b667b8405d8934c0ed8463e3273f3e4c22c989/src/wp-includes/load.php#L394-L400