types(remix-server-runtime): make `AppLoadContext` an empty interface instead of `any` #1876

DanielFGray · 2022-02-10T20:48:42Z

this allows users to override the context with their own definitions using ambient modules.

as an example:

declare module "remix" {
  interface AppLoadContext {
    graphql: <Result, Variables>(operation: {
      query: TypedDocumentNode<Result, Variables>
      variables: Variables
    }) => Promise<ExecutionResult<Result>>
  }
}

remix-cla-bot · 2022-02-10T20:48:45Z

Hi @DanielFGray,

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

ryanflorence · 2022-02-10T20:55:56Z

What does this do if you don't add that declare? Does it treat it as any or are you required to declare it?

remix-cla-bot · 2022-02-10T21:01:16Z

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

DanielFGray · 2022-02-10T21:05:40Z

What does this do if you don't add that declare?

do you mean the declare in my example? it's my understanding that it's not a valid ambient module declaration without it

Does it treat it as any or are you required to declare it?

forgive me, I'm not sure I understand the question.

currently it's impossible to override the type for context that is passed to loader functions etc since AppLoadContext is defined as any, but defining it as an empty interface allows it to be an object that can be typed with ambient modules

itsMapleLeaf · 2022-02-10T21:14:04Z

What does this do if you don't add that declare? Does it treat it as any or are you required to declare it?

If you don't add the declare, the type of context will be {}. This PR allows you to add a declare to change the context type to whatever you want. This is similar to declaring a type to extend the express request: https://stackoverflow.com/a/40762463/1332403

0x221A · 2022-02-10T21:27:15Z

This is how I currently override the type for context it's quite hard btw. I'm agreed with this PR since typescript will merge the interface with the same name within a module.

export interface AppLoadContext {}

/**
 * The arguments passed to ActionFunction and LoaderFunction.
 */
export type DataFunctionArgs<T extends (...args: any) => any> = Omit<Parameters<T>[0], 'context'> & {
  context: AppLoadContext
}
/**
 * A function that handles data mutations for a route.
 */
export interface ActionFunction {
  (args: DataFunctionArgs<RemixActionFunction>): ReturnType<RemixActionFunction>
}

ryanflorence · 2022-02-10T21:49:42Z

Sorry, I'm not as well-versed in TypeScript as you might expect, I just know enough to get by.

I understand this PR enables you to do this:

declare module "remix" {
  interface AppLoadContext {
    graphql: <Result, Variables>(operation: {
      query: TypedDocumentNode<Result, Variables>
      variables: Variables
    }) => Promise<ExecutionResult<Result>>
  }
}

What if in my app I don't declare anything and I add load context and try to access it, what will the type be?

export let loader: LoaderFunction = async ({ context }) => {
  // what is context? Any? Can I access stuff on it w/o typescript getting mad?
  console.log("is TS mad?", context.foo);
}

I'd like TypeScript to treat it as any.

Another way to ask my question: does this PR now require an app to declare the AppLoadContext in order to use it without TypeScript yelling at you?

0x221A · 2022-02-10T21:56:08Z

What if in my app I don't declare anything and I add load context and try to access it, what will the type be?

That should be undefined since this just allows us to work with types easier. It's like javascript if the key doesn't exist in the object so that value should be undefined

ryanflorence · 2022-02-10T22:17:28Z

I don't know why this question is so hard for me to ask and get an answer that makes sense to me 😅

With this PR, do I get red squiggly lines under context.foo if I don't declare an app load context type?

airjp73 · 2022-02-10T22:34:11Z

I don't know why this question is so hard for me to ask and get an answer that makes sense to me 😅

With this PR, do I get red squiggly lines under context.foo if I don't declare an app load context type?

I think the answer is yes. Typescript playground example

ryanflorence · 2022-02-10T22:44:24Z

Do you have to have an empty interface to be able to do this or can you do it if it's typed as any?

declare module "remix" {
  interface AppLoadContext {
    /* type here */
  }
}

airjp73 · 2022-02-10T22:54:11Z

There are other things like namespaces that support declaration merging, but type does not. You get a "duplicate identifier" error (typescript playground).

0x221A · 2022-02-10T22:55:43Z

Do you have to have an empty interface to be able to do this or can you do it if it's typed as any?

You need to do that only if you want to add new properties to an interface. After that Typescript will merge the properties after you declare.

itsMapleLeaf · 2022-02-10T22:57:10Z

Do you have to have an empty interface to be able to do this or can you do it if it's typed as any?

You cannot do what this PR wants if it's typed as any. Updating a library's types via declare module only works if the type being updated is an interface, and interfaces can't be any.

However, updating the PR with this would be close enough:

export interface AppLoadContext {
  [key: string]: any;
};

This would let you do context.something without errors, and allow it to be updated with declare module.

Slight downside: declare module can't erase the any, so the example in the OP would still let you do context.something without error. But I'd be willing to live with that.

0x221A · 2022-02-10T23:00:21Z

This PR is just for better DX since esbuild doesn't care what you declare in the type system 😂

ryanflorence · 2022-02-10T23:01:48Z

@itsMapleLeaf ah, okay, I can maybe live with that? Don't like that it has to be an object now, but seems weird to be anything else.

@mjackson you have an opinion here?

0x221A · 2022-02-10T23:12:11Z

The other way is leaving it be any and using a passed-in type to the loader() and action() function but I don't see why to do that because AppLodeContext came from the server. Just declare once and use it anywhere.

3x071c · 2022-03-24T15:30:42Z

@ryanflorence Just came here after almost having opened a similar PR myself. This is the signature for AppLoadContext I'd recommend instead of an any type, which doesn't allow for declaration merging and therefore can't be strongly typed:

export declare interface AppLoadContext {
	[key: string]: unknown
};

This will continue allowing accessing any properties on AppLoadContext by default, but prevents unsafe property access by requiring type assertions at runtime. For those that prefer strict typing and know the structure of their context in advance, they can merge this generic interface declaration with their own.

MichaelDeBoey · 2022-03-28T16:27:39Z

@DanielFGray Code/type changes go against dev.
Could you please rebase your branch onto latest dev & set base branch to dev + resolve conflicts?

DanielFGray · 2022-04-20T21:55:59Z

I have updated this to use unknown, per @3x071c's suggestion

@MichaelDeBoey I think I rebased this properly

marwan38 · 2022-05-27T03:35:19Z

Looks like it includes changes that weren't in this PR. Might want to reflog and or rebase again. Looking forwards to this being merged in as we have a similar use case.

MichaelDeBoey · 2022-05-27T07:39:12Z

@DanielFGray Like @marwan38 said: the rebase included changes that doesn't belong in this PR.
Please remove these from your branch/PR.

justinwaite · 2022-07-15T02:55:00Z

@MichaelDeBoey just out of curiosity, at what point could we consider this stale or abandoned and give someone else a chance to get it across the finish line? I think this is a pretty large impact item for our company since our apps will all rely on this context. Let me know, I'm definitely willing to help!

changeset-bot · 2022-07-15T13:41:08Z

🦋 Changeset detected

Latest commit: 33fd91b

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/netlify	Major
@remix-run/server-runtime	Major
@remix-run/cloudflare	Major
@remix-run/deno	Major
@remix-run/dev	Major
@remix-run/node	Major
@remix-run/cloudflare-pages	Major
@remix-run/cloudflare-workers	Major
create-remix	Major
@remix-run/architect	Patch
@remix-run/express	Major
@remix-run/vercel	Major
@remix-run/serve	Major
remix	Major
@remix-run/eslint-config	Major
@remix-run/react	Major

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

justinwaite · 2022-07-15T14:06:45Z

packages/remix-server-runtime/server.ts

@@ -16,7 +16,7 @@ import { createServerHandoffString } from "./serverHandoff";

 export type RequestHandler = (
  request: Request,
-  loadContext?: AppLoadContext
+  loadContext: AppLoadContext


Should this remain optional?

Suggested change

loadContext: AppLoadContext

loadContext?: AppLoadContext

justinwaite · 2022-07-16T00:14:14Z

packages/remix-server-runtime/data.ts

@@ -18,7 +20,7 @@ export async function callRouteAction({
  match,
  request,
 }: {
-  loadContext: unknown;
+  loadContext: AppLoadContext;
