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

ESLint: Fix a couple of React Compiler reassignment errors #66331

Merged
merged 1 commit into from
Oct 24, 2024
Merged

ESLint: Fix a couple of React Compiler reassignment errors #66331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 8 additions & 6 deletions packages/block-editor/src/components/list-view/branch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
__experimentalTreeGridRow as TreeGridRow,
__experimentalTreeGridCell as TreeGridCell,
} from '@wordpress/components';
import { memo } from '@wordpress/element';
import { memo, useRef } from '@wordpress/element';
import { AsyncModeProvider, useSelect } from '@wordpress/data';

/**
Expand Down Expand Up @@ -123,6 +123,8 @@ function ListViewBranch( props ) {
draggedClientIds,
} = useListViewContext();

const nextPositionRef = useRef();

if ( ! canParentExpand ) {
return null;
}
Expand All @@ -133,15 +135,15 @@ function ListViewBranch( props ) {
const blockCount = filteredBlocks.length;
// The appender means an extra row in List View, so add 1 to the row count.
const rowCount = showAppender ? blockCount + 1 : blockCount;
let nextPosition = listPosition;
nextPositionRef.current = listPosition;
Copy link
Member

Choose a reason for hiding this comment

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

This probably goes against the "don't read/write refs during render" rule, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it might, but it doesn't really trigger an error. And I think we can find similar usage across the repo already, so 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why exactly the compiler didn't like the nextPosition local variable. Maybe it's that it's incremented in a map callback and the compiler thinks that the callback is an event handler?

nextPosition is a legit local variable, assigned and used only during the render, and the value doesn't need to survive the render function. The ref is not really needed. Reading/writing it is safe because, well, it's used as a local variable.


return (
<>
{ filteredBlocks.map( ( block, index ) => {
const { clientId, innerBlocks } = block;

if ( index > 0 ) {
nextPosition += countBlocks(
nextPositionRef.current += countBlocks(
filteredBlocks[ index - 1 ],
expandedState,
draggedClientIds,
Expand All @@ -165,7 +167,7 @@ function ListViewBranch( props ) {
} );

const { itemInView } = fixedListWindow;
const blockInView = itemInView( nextPosition );
const blockInView = itemInView( nextPositionRef.current );

const position = index + 1;
const updatedPath =
Expand Down Expand Up @@ -218,7 +220,7 @@ function ListViewBranch( props ) {
showBlockMovers={ showBlockMovers }
path={ updatedPath }
isExpanded={ isDragged ? false : shouldExpand }
listPosition={ nextPosition }
listPosition={ nextPositionRef.current }
selectedClientIds={ selectedClientIds }
isSyncedBranch={ syncedBranch }
displacement={ displacement }
Expand All @@ -239,7 +241,7 @@ function ListViewBranch( props ) {
showBlockMovers={ showBlockMovers }
level={ level + 1 }
path={ updatedPath }
listPosition={ nextPosition + 1 }
listPosition={ nextPositionRef.current + 1 }
fixedListWindow={ fixedListWindow }
isBranchSelected={ isSelectedBranch }
selectedClientIds={ selectedClientIds }
Expand Down
10 changes: 5 additions & 5 deletions packages/block-library/src/table-of-contents/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
ToolbarGroup,
} from '@wordpress/components';
import { useDispatch, useSelect } from '@wordpress/data';
import { renderToString } from '@wordpress/element';
import { renderToString, useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { useInstanceId } from '@wordpress/compose';
import { store as noticeStore } from '@wordpress/notices';
Expand Down Expand Up @@ -59,14 +59,14 @@ export default function TableOfContentsEdit( {

// If a user clicks to a link prevent redirection and show a warning.
const { createWarningNotice, removeNotice } = useDispatch( noticeStore );
let noticeId;
const noticeIdRef = useRef();
const showRedirectionPreventedNotice = ( event ) => {
event.preventDefault();
// Remove previous warning if any, to show one at a time per block.
removeNotice( noticeId );
noticeId = `block-library/core/table-of-contents/redirection-prevented/${ instanceId }`;
removeNotice( noticeIdRef.current );
noticeIdRef.current = `block-library/core/table-of-contents/redirection-prevented/${ instanceId }`;
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 need reassignment here? The instanceId shouldn't change between rerenders; we can always use clientId if it does.

P.S. The Latest Posts block has similar logic.

gutenberg/packages/block-library/src/latest-posts/edit.js

Lines 159 to 169 in acf661c

let noticeId;
const showRedirectionPreventedNotice = ( event ) => {
event.preventDefault();
// Remove previous warning if any, to show one at a time per block.
removeNotice( noticeId );
noticeId = `block-library/core/latest-posts/redirection-prevented/${ instanceId }`;
createWarningNotice( __( 'Links are disabled in the editor.' ), {
id: noticeId,
type: 'snackbar',
} );
};

Copy link
Member

Choose a reason for hiding this comment

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

Does the version in Latest Posts really work? It seems to me that the noticeId is reset to undefined on each rerender of the block's Edit component. When you click and the showRedirectionPreventedNotice handler is called, the created notice id is saved, but on the next rerender it's lost again.

Copy link
Member

Choose a reason for hiding this comment

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

Dispatching a notice won't rerender the Edit component, so the noticeId is retained. But yes, the value might get lost.

By the way, the following snippet has the same effect and displays only a single notice.

const showRedirectionPreventedNotice = ( event ) => {
	event.preventDefault();
	createWarningNotice( __( 'Links are disabled in the editor.' ), {
		id: `block-library/core/latest-posts/redirection-prevented/${ instanceId }`,
		type: 'snackbar',
	} );
};

Copy link
Member

Choose a reason for hiding this comment

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

Dispatching a notice won't rerender, but if the component is rerendered for any other reason then the noticeId value will be lost. Adding the ref is a correct fix, and it should be applied also to the LatestPostsEdit component.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @jsnajdr that the ref is the correct fix.

I'm happy to follow up with fixing that in LatestPostsEdit if we all agree on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Followed up here for addressing in the Latest Posts block: #66404

Copy link
Member Author

Choose a reason for hiding this comment

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

Even better follow-up here: #66409

createWarningNotice( __( 'Links are disabled in the editor.' ), {
id: noticeId,
id: noticeIdRef.current,
type: 'snackbar',
} );
};
Expand Down
Loading