-
Notifications
You must be signed in to change notification settings - Fork 68
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
fixes #4108 Forum returns 404 for correct links #4318
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
@IlyaSmiyukha to QA this and simulate the QN going down without needing to run a local network I used FoxyProxy.
But the page still redirect to a 404 instead of showing an unending loading screen or an error message. Which would both be an improvement on the current behavior:
Peek.2023-04-07.16-59.webm
Will check it |
I made a fix for handling errors for threads. Right now it shows an infinity loader. We can change it to an error message like this. But I need a text for the message. |
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.
The loader would be a good option if Apollo was re-trying on error which doesn't seem to be the case (maybe there is a setting for it).
Alternatively could we have something like:
"Something went wrong fetching this thread: {Error message returned by the QN}"
WDYT @dmtrjsg ?
@@ -38,6 +38,7 @@ export const useRefetchQueries = ( | |||
}, interval) | |||
|
|||
return () => { | |||
isRefetched.current = false |
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.
@IlyaSmiyukha I don't think this change is right. AFAIK isRefetched
is just supposed to indicate that the data was already fetched once. So once it has been set to true
it should remain this way. Or am I missing something ?
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.
For the current post you are right, But when the user opens another post isRefetched
will return true. And in this case he will be redirected to 404 page. So when the URL was changed we should update isRefetched as well
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 see... But the problem with this solution is that: in places where deps
is undefined
(which is most places the hook is used), the useEffect
is re-rendered periodically. As a result isRefetched
becomes false
periodically.
isRefetched
was introduced in #3359 to prevent the loading screen to reappear when re-fetching data. So setting to false
at the wrong time will cause this issue.
Here whether this issue will happen depends on whether the data are re-fetched due to a re-render or the interval and the re-render + the time it takes to actually re-fetch the data. Which makes the behavior of this hook hard to predict.
For example when testing while simulating the QN being down it happened for me on the main forum page:
Peek.2023-04-14.10-32.webm
The page only flickers because the request fails quickly.
So this solution should be improved a bit. However I wasn't able to reproduce the case you described. Here's my attempt:
Peek.2023-04-14.11-15.webm
In my understanding when the URL is changed the hook is unmounted and remounted so isRefetched
is reset to false
.
Or am I testing the wrong case ?
@thesan sounds good 👍 |
I pushed changes. It will say: Something went wrong fetching this thread. Otherwise, it will look a bit ugly with {an Error message returned by the QN}. Something went wrong fetching this thread: Error: failed to fetch |
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.
@@ -38,6 +38,7 @@ export const useRefetchQueries = ( | |||
}, interval) | |||
|
|||
return () => { | |||
isRefetched.current = false |
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 see... But the problem with this solution is that: in places where deps
is undefined
(which is most places the hook is used), the useEffect
is re-rendered periodically. As a result isRefetched
becomes false
periodically.
isRefetched
was introduced in #3359 to prevent the loading screen to reappear when re-fetching data. So setting to false
at the wrong time will cause this issue.
Here whether this issue will happen depends on whether the data are re-fetched due to a re-render or the interval and the re-render + the time it takes to actually re-fetch the data. Which makes the behavior of this hook hard to predict.
For example when testing while simulating the QN being down it happened for me on the main forum page:
Peek.2023-04-14.10-32.webm
The page only flickers because the request fails quickly.
So this solution should be improved a bit. However I wasn't able to reproduce the case you described. Here's my attempt:
Peek.2023-04-14.11-15.webm
In my understanding when the URL is changed the hook is unmounted and remounted so isRefetched
is reset to false
.
Or am I testing the wrong case ?
Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>
@thesan |
Screen.Recording.2023-04-15.at.11.25.13.mov@thesan |
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.
Hey @IlyaSmiyukha you were right there was a lot going wrong with the query refetching: First the isRefetched
was set to true
before it got fetched in the first place, second there is no GetForumThreads
query on this page and finally the posts should be polled not the thread itself. Also the useRefetchQueries
is called almost everywhere without dependencies, which works but is bad for performances, but then I think that's the intended behavior in some places, so it shouldn't be changed everywhere blindly.
Since there was a lot to fix I committed the fixes directly on this PR. Hopefully it doesn't break other component using the same hook 🤞
Good job catching the bug here 🙌
@chrlschwb could someone give this PR one last review before I merge it please? Just in case I missed something in the code I wrote.
It's great! Thanks @thesan |
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.
All the stuff you mentioned are in the code.
fixes #4108