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

[justinwaite]

Closes: #3988

My strategy for addressing this issue is to always pass an empty object for the context, rather than passing nothing (or undefined). Since we've already declared the AppLoadContext interface to be {[key: string]: unknown}, I feel like this would be a reasonable approach.

• Docs
• Tests

Testing Strategy:

I created an app in the playground and (manually had to) copy over the built remix-server-runtime directory into it. I then augmented the AppLoadContext interface with a property called notes. I then tried to access both a known and unknown property to make sure that typescript knew the difference.

declare module '@remix-run/server-runtime' {
  export interface AppLoadContext {
    notes: string[]
  }
}

export const loader: LoaderFunction = async ({ request, context }) => {
  console.log(context.asdf.toString());
  
  console.log(context.notes.map(n => n.toLowerCase())); // Look Ma! No undefined check for context!

  let userId = await requireUserId(request);
  let noteListItems = await getNoteListItems({ userId });
  return json<LoaderData>({ noteListItems });
};

Here you can see the difference. Since asdf isn't part of the AppLoadContext augmented interface, Typescript correctly infers it as unknown. However, since notes was added to the interface, it correctly infers it as a string[]. Note in either case that we didn't need to add an undefined check for the context, since it will always be at least an empty object ({}).

[changeset-bot]

🦋 Changeset detected

Latest commit: dcc1a57

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@remix-run/server-runtime	Patch
@remix-run/cloudflare	Patch
@remix-run/deno	Patch
@remix-run/dev	Patch
@remix-run/node	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
create-remix	Patch
@remix-run/architect	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/vercel	Patch
@remix-run/serve	Patch
remix	Patch
@remix-run/eslint-config	Patch
@remix-run/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[remix-cla-bot]

Hi @justinwaite,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[marwan38]

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

[justinwaite]

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

[pcattori]

Looks like this is on the right track!

We'd like to make all user-facing APIs (i.e. things exported from the main index.ts) optional (context?: AppLoadContext) while making all the internal APIs required (context: AppLoadContext).

The only user-facing API should be the requestHandler function within the createRequestHandler function. I think all that's needed is to change that to:

  return async function requestHandler(request, loadContext = {}) {

For all other places, you should be able to make the load context required.

[justinwaite]

Looks like this is on the right track!

We'd like to make all user-facing APIs (i.e. things exported from the main index.ts) optional (context?: AppLoadContext) while making all the internal APIs required (context: AppLoadContext).

The only user-facing API should be the requestHandler function within the createRequestHandler function. I think all that's needed is to change that to:

  return async function requestHandler(request, loadContext = {}) {

For all other places, you should be able to make the load context required.

Hey @pcattori, what exactly would you like me to change here? The interface for RequestHandler remains the same:

export type RequestHandler = (
  request: Request,
  loadContext?: AppLoadContext
) => Promise<Response>;

The only place I'm making it required is in callRouteAction and callRouteLoader. The other changes I'm just using some nullish coalescing to ensure that something is passed in, i.e. an empty object, in the event that the user passes null or undefined into the requestHandler for the context.

[pcattori]

Hey @pcattori, what exactly would you like me to change here?

I think @marwan38 said what should change more clearly than I did 😅 was just trying to emphasize that we want that change not only because its tidier, but because it correctly models that users should be able to omit loadContext arg and requestHandler is the only place users would do that.

[pcattori]

Waiting for #4010 to fix the Windows CI errors

[marwan38]

Nice! Thank you