-
Notifications
You must be signed in to change notification settings - Fork 188
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
Handle 404 properly #1179
Handle 404 properly #1179
Conversation
Looks like it's not going to be possible to detect 404 on Task Search right now. @tpetr @kwm4385 @wolfd This PR is good to be reviewed/merged from my perspective now. |
url: `/history/task/${taskId}` | ||
(taskId, isMainApiCall) => ({ | ||
url: `/history/task/${taskId}`, | ||
isMainApiCall |
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.
What does this mean?
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.
Unfortunately the meaning snuck into another PR - that's on me, I implemented it with the Deploy Detail page fixes.
It's basically an idiomatic way of saying it ignores 404s. It signifies that if the API call fails with a 404 then the page should 404 (but it can also be extended to mean other things later).
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.
isMainApiCall
doesn't convey that information at all -- if that's all that this variable controls, can we think of a more descriptive name for it?
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.
Naming things isn't my strong suit. Maybe pageCantRenderIfThisFails
? I don't really like that name either though.
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.
Or page404sIfThis404s
.
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.
It's cool, naming is hard 😄 -- I'd suggest something like renderNotFoundIf404
LGTM |
1 similar comment
LGTM |
Handles 404s on pages for specific requests, deploys, and tasks by showing the 404 page rather than rendering something meaningless and showing an error.
The following pages are affected:
Please let me know if I've missed any @tpetr @kwm4385 @wolfd