-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
initial work on clientLoader & clientAction #5543
Conversation
|
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.
So pumped for this 🤩
packages/remix-react/data.ts
Outdated
@@ -29,7 +29,8 @@ export function isErrorResponse(response: any): boolean { | |||
export function isRedirectResponse(response: any): boolean { | |||
return ( | |||
response instanceof Response && | |||
response.headers.get("X-Remix-Redirect") != null | |||
(response.headers.get("X-Remix-Redirect") != null || | |||
(response.status >= 300 && response.status < 400)) |
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.
We can steal this util from RR (or even export it as UNSAFE_
and use it directly:
const redirectStatusCodes = new Set([301, 302, 303, 307, 308]);
export function isRedirectStatusCode(statusCode: number): boolean {
return redirectStatusCodes.has(statusCode);
}
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.
Is the reason we need this because clientLoader doesn't run through the internal checking? Maybe we can get rid of the need for this an put less in the doFetch
...
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 reason we need this is because Remix is expecting a 204 response to represent redirects and we don't currently handle real redirect responses.
packages/remix-react/routeModules.ts
Outdated
export interface ClientDataFunctionArgs { | ||
request: Request; | ||
params: Params; | ||
next: () => Promise<Response | DeferredData | null>; |
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.
We can bikeshed this as we go - I like serverFetch
at the moment
let doFetch = async (request: Request) => { | ||
if (isAction && !route.hasAction) { | ||
let msg = | ||
`Route "${route.id}" does not have an action, but you are trying ` + | ||
`to submit to it. To fix this, please add an \`action\` function to the route`; | ||
console.error(msg); | ||
throw new Error(msg); | ||
} else if (!isAction && !route.hasLoader) { | ||
return null; | ||
} | ||
|
||
let result = await fetchData(request, route.id); |
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 wonder if doFetch
could stop here so we get all the same error/redirect/catch handling whether they hit the server or not?
I'm gonna close this out for now - we can re-open if needed once we start on v3 features |
Closes: #5070
Testing Strategy: