Skip to content
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

Jump to bottom

types(remix-server-runtime): make context mandatory in DataFunctionArgs #3989

Merged
merged 3 commits into from
Aug 17, 2022
Merged

types(remix-server-runtime): make context mandatory in DataFunctionArgs #3989

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0fa88c
fix(AppLoadContext): remove optionality from AppLoadContext in DataFu…
justinwaite Aug 12, 2022
71a9e1e
refactor: move the loadContext object default higher up the chain
justinwaite Aug 17, 2022
dcc1a57
Merge branch 'dev' into dev
pcattori Aug 17, 2022
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
5 changes: 5 additions & 0 deletions .changeset/thirty-pots-lay.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/server-runtime": patch
---

Make the loadContext parameter not optional so that augmenting AppLoadContext does not require checking if the context is undefined
1 change: 1 addition & 0 deletions contributors.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@
- juliaqiuxy
- justinnoel
- justinsalasdev
- justinwaite
- justsml
- juwiragiye
- jveldridge
Expand Down
4 changes: 2 additions & 2 deletions packages/remix-server-runtime/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export async function callRouteAction({
match,
request,
}: {
loadContext?: AppLoadContext;
loadContext: AppLoadContext;
match: RouteMatch<ServerRoute>;
request: Request;
}) {
Expand Down Expand Up @@ -67,7 +67,7 @@ export async function callRouteLoader({
}: {
request: Request;
match: RouteMatch<ServerRoute>;
loadContext?: AppLoadContext;
loadContext: AppLoadContext;
}) {
let loader = match.route.module.loader;

Expand Down
2 changes: 1 addition & 1 deletion packages/remix-server-runtime/routeModules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export interface RouteModules<RouteModule> {
*/
export interface DataFunctionArgs {
request: Request;
context?: AppLoadContext;
context: AppLoadContext;
params: Params;
}

Expand Down
14 changes: 7 additions & 7 deletions packages/remix-server-runtime/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ async function handleDataRequest({
match = getRequestMatch(url, matches);

response = await callRouteAction({
loadContext,
loadContext: loadContext ?? {},
Copy link

@marwan38 marwan38 Aug 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would default this as high up as possible. So, in the args definition line 79) or even higher up. I'm assuming that the consumer of this function doesn't really or shouldn't be instantiating the empty object. In that case malang it okay to default it as an argument.

Edit: same with all the other changes. It's just a little bit more elegant. I wonder if defining it at the start of thr chain is possible or a "cleaner" solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I moved it up to the requestHandler to be the default value for the loadContext arg. This definitely did clean it up

match,
request: request,
});
Expand All @@ -129,7 +129,7 @@ async function handleDataRequest({
}
match = tempMatch;

response = await callRouteLoader({ loadContext, match, request });
response = await callRouteLoader({ loadContext: loadContext ?? {}, match, request });
}

if (isRedirectResponse(response)) {
Expand All @@ -151,7 +151,7 @@ async function handleDataRequest({

if (handleDataRequest) {
response = await handleDataRequest(response, {
context: loadContext,
context: loadContext ?? {},
params: match.params,
request,
});
Expand Down Expand Up @@ -225,7 +225,7 @@ async function handleDocumentRequest({

try {
actionResponse = await callRouteAction({
loadContext,
loadContext: loadContext ?? {},
match: actionMatch,
request: request,
});
Expand Down Expand Up @@ -302,7 +302,7 @@ async function handleDocumentRequest({
matchesToLoad.map((match) =>
match.route.module.loader
? callRouteLoader({
loadContext,
loadContext: loadContext ?? {},
match,
request: loaderRequest,
})
Expand Down Expand Up @@ -524,9 +524,9 @@ async function handleResourceRequest({

try {
if (isActionRequest(request)) {
return await callRouteAction({ match, loadContext, request });
return await callRouteAction({ match, loadContext: loadContext ?? {}, request });
} else {
return await callRouteLoader({ match, loadContext, request });
return await callRouteLoader({ match, loadContext: loadContext ?? {}, request });
}
} catch (error: any) {
if (serverMode !== ServerMode.Test) {
Expand Down