Skip to content

ServerSideRender: Fix data loading in development mode #62140

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

Merged
Mamaduka merged 3 commits into from
May 30, 2024
Merged

ServerSideRender: Fix data loading in development mode #62140

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
013ca23
ServerSideRender: Fix data loading in development mode
Mamaduka May 30, 2024
0ff8277
Colocate the loading state handlers
Mamaduka May 30, 2024
84fb8c9
Remove leftover testing code
Mamaduka May 30, 2024
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
35 changes: 14 additions & 21 deletions packages/server-side-render/src/server-side-render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export default function ServerSideRender( props ) {
LoadingResponsePlaceholder = DefaultLoadingResponsePlaceholder,
} = props;

const isMountedRef = useRef( true );
const isMountedRef = useRef( false );
Copy link
Member

@tyxla tyxla May 30, 2024

Choose a reason for hiding this comment

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

But it did mount already, didn't it? Unless I misunderstand something, this looks wrong - can you elaborate?

Copy link
Member Author

@Mamaduka Mamaduka May 30, 2024

Choose a reason for hiding this comment

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

Setting the initial value doesn't mean that the component was mounted. React can call a component (function) for other reasons without mounting it.

This is a common (anti)pattern when you want to run synchronization only after the initial mount.

const didMount = useRef( false );
useEffect( () => {
   didMount.current = true;
   return () => { didMount.current = false };
}, [] );

Copy link
Member Author

@Mamaduka Mamaduka May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tyxla, I think this is the third issue I fixed for a similar pattern. I explained why the original implementations were failing in development mode here: #62141 (comment).

Copy link
Member

@tyxla tyxla May 30, 2024

Choose a reason for hiding this comment

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

Ah, of course, got it. Wow, this component is messy 😉

Copy link
Member Author

@Mamaduka Mamaduka May 30, 2024

Choose a reason for hiding this comment

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

Yeah, it's bit of noodle soup 🍜

const [ showLoader, setShowLoader ] = useState( false );
const fetchRequestRef = useRef();
const [ response, setResponse ] = useState( null );
Expand All @@ -112,6 +112,11 @@ export default function ServerSideRender( props ) {

setIsLoading( true );

// Schedule showing the Spinner after 1 second.
const timeout = setTimeout( () => {
setShowLoader( true );
}, 1000 );

let sanitizedAttributes =
attributes &&
__experimentalSanitizeBlockAttributes( block, attributes );
Expand Down Expand Up @@ -165,6 +170,9 @@ export default function ServerSideRender( props ) {
fetchRequest === fetchRequestRef.current
Copy link
Member

@ellatrix ellatrix May 30, 2024

Choose a reason for hiding this comment

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

I guess to be completely correct, we should cancel the fetch request if the component unmounts? Then we also don't need a mount ref?

Copy link
Member Author

@Mamaduka Mamaduka May 30, 2024

Choose a reason for hiding this comment

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

I tried only to change what was necessary for the fix + minor colocation.

If you ask me, this whole component needs proper refactoring 😅

) {
setIsLoading( false );
// Cancel the timeout to show the Spinner.
setShowLoader( false );
Copy link
Member

@tyxla tyxla May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to setShowLoader( false ) on cleanup? Feels odd to set the state on unmounting, since we won't be using that state.
Scratch this, I was looking at the wrong place and thought this was running in an effect.

clearTimeout( timeout );
}
} ) );

Expand All @@ -175,12 +183,12 @@ export default function ServerSideRender( props ) {

// When the component unmounts, set isMountedRef to false. This will
// let the async fetch callbacks know when to stop.
useEffect(
() => () => {
useEffect( () => {
isMountedRef.current = true;
return () => {
isMountedRef.current = false;
},
[]
);
};
}, [] );

useEffect( () => {
// Don't debounce the first fetch. This ensures that the first render
Expand All @@ -192,21 +200,6 @@ export default function ServerSideRender( props ) {
}
} );

/**
* Effect to handle showing the loading placeholder.
* Show it only if there is no previous response or
* the request takes more than one second.
*/
useEffect( () => {
if ( ! isLoading ) {
return;
}
const timeout = setTimeout( () => {
setShowLoader( true );
}, 1000 );
return () => clearTimeout( timeout );
}, [ isLoading ] );

const hasResponse = !! response;
const hasEmptyResponse = response === '';
const hasError = response?.error;
Expand Down