Refactor ServerSideRender to use React hooks. #28297
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZebulanStanphill
added
[Type] Code Quality
|
Size Change: -266 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
ZebulanStanphill
force-pushed
the
update/server-side-render-hooks-refactor
branch
2 times, most recently
from
b6c530e
to
30ccdb6
Compare
fabiankaegy
requested changes
Jan 19, 2021
ZebulanStanphill
force-pushed
the
update/server-side-render-hooks-refactor
branch
2 times, most recently
from
785c9d3
to
b0cd3c7
Compare
ZebulanStanphill
force-pushed
the
update/server-side-render-hooks-refactor
branch
from
b0cd3c7
to
6cf8653
Compare
kevin940726
reviewed
Mar 10, 2021
Comment on lines
+123
to
+140
| // When the component unmounts, set isMountedRef to false. This will | ||
| // let the async fetch callbacks know when to stop. | ||
| useEffect( | ||
| () => () => { | ||
| isMountedRef.current = false; | ||
| }, | ||
| [] | ||
| ); | ||
|
|
||
| useEffect( () => { | ||
| // Don't debounce the first fetch. This ensures that the first render | ||
| // shows data as soon as possible | ||
| if ( prevProps === undefined ) { | ||
| fetchData(); | ||
| } else if ( ! isEqual( prevProps, props ) ) { | ||
| debouncedFetchData(); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Nitpicking: I think we can potentially tidy these up into just one effect. IMHO it's clearer and we can save the need for comparing isEqual on every render.
const isMountedRef = useRef(false);
useEffect(() => {
function fetchData() {
// ...
}
// Initial request
if (!isMountedRef.current) {
fetchData();
}
isMountedRef.current = true;
const debounceFetchData = debounce(fetchData, 500);
// Debounced request
debounceFetchData();
return () => {
isMountedRef.current = false;
}
}, [...])It's not tested so I might have missed something though.
There was a problem hiding this comment.
I don't think the suggested code would work:
- It would cause
fetchDatato be called twice on the first render. - It would cause
debounceFetchDatato be called on every render, which would causeresponseto be set on every render, thus causing an infinite loop, if I had to guess. The comparison against the previous props exists to prevent this.
There was a problem hiding this comment.
Ahh, yeah, you're right, I missed that part. I still believe we can somehow skip the isEqual comparison though, but that's not very important anyway.
There was a problem hiding this comment.
There's a pretty good article I found recently discussing this kinda stuff:
https://www.benmvp.com/blog/object-array-dependencies-react-useEffect-hook/
In this case, the useEffect callback is dependent upon attributes, block, urlQueryArgs, and httpMethod; the first 3 of those are objects (meaning we can't just use shallow equality checks), and using destructuring on the first 2 won't work since their properties may themselves be objects.
As suggested in the article, a useDeepCompareEffect hook would be the best way to solve the problem, but that would probably only be a slight improvement over what I'm already doing, and since it introduces an external dependency (albeit a small one), I think it might be better suited to a separate PR.
There was a problem hiding this comment.
Yeah, I think this is good enough, and we're just refactoring it to hooks anyway. 👍
kevin940726
approved these changes
Mar 12, 2021
|
I went ahead and did a quick test again and it seems like everything is working as intended. @fabiankaegy You need to dismiss your review or switch to an approving review before I can merge. For some reason I can't find a way to do the former. Once this is merged, #28289 can be rebased and will be a lot smaller and easier to review, which should hopefully help it to land in core sooner. |
fabiankaegy
approved these changes
Mar 12, 2021
|
@fabiankaegy The problem with this change is that if you change a prop (block option) during the loading stage (while the spinner is showing), the entire content of the block disappears. Beforebefore.mp4Afterafter.mp4FYI: You'll notice in the "after" screen recording that the layout of the block changes during loading. This is because I have applied the |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Part of #22890. This PR refactors the
ServerSideRendercomponent into a function component using React hooks.Getting this merged will make future changes like #28289 easier to land.
How has this been tested?
I inserted an Archives block (which uses
ServerSideRender) and verified that changing the options in the toolbar and inspector updated the rendered block in the editor.Checklist: