[RFC] Change how queries/mutations access ctx and how middleware is typed

[flybayer]

Currently queries/mutations access ctx as shown below. Data and functions can be added to ctx via HTTP Middleware. The primary use case for this is authentication and authorization.

import { SessionContext } from "blitz";

export default async function getUser(
  { where }: GetUserInput,
  ctx: { session?: SessionContext } = {}
) {
  ctx.session!.authorize();

  const user = await db.user.findOne({ where });

  return user;
}

An you can "chain" resolvers together like this (pass ctx as second arg).

import createUser from "./createUser";

export default async function createUsers(
  { users },
  ctx: { session?: SessionContext } = {}
) {
  for (const user of users) {
    await createUser(user, ctx);
  }
}

There are a few problems with this that necessitates a change.

1. You have to type ctx.session as optional. Because if you make it required, then when using await getUser() on the frontend, TS compilation will fail. And you can't add ctx there because that's on the client. So this is quite annoying to always deal with.
2. Security issue: Because of that type issue, you have to use ctx.session!.authorize() to tell TS that yes ctx.session will be here. However, when typing that out, text editors will automatically write it as ctx.session?.authorize() which is a big problem. Because when written with ? instead of ! requests will not be authorized if ctx.session is undefined for some reason.
3. Lastly, to be properly typed, ctx.session.authorize() should function as a TS assertion function so that after calling that, the TS type tells you that ctx.session.userId is not null. Normally this is possible, but when called with the non-null assertion (!) the assertion function does not work correctly.

Requirements

1. Clean type signature when using from React components (it should not let you pass a second argument to the query/mutation)
2. Strong types inside queries/mutations. I.e. ctx.session should be SessionContext, not SessionContext | undefined
3. Ability to directly provide the context to a another resolver. For example, ability to have an uploadUsersCsv() mutation that directly imports the createUser() mutation and passes ctx to it.
4. Easily type all global middleware

New Proposals

I've come up with two alternate proposals that both significantly improve over the existing approach. If you can think of something better than these, please share it!

1. HOF Method

We can add a higher order function (HOF) that will add the types for you. The resulting function type is overloaded so that you can call the resolver without ctx from react components and with ctx when calling directly on the server.

import { resolver } from "blitz";

export default resolver(async function getUser({ where }: GetUserInput, ctx) {
  ctx.session.authorize();

  const user = await db.user.findOne({ where });

  return user;
});

And you would chain resolvers like this:

import { resolver } from "blitz";
import createUser from "./createUser";

export default resolver(async function createUsers({ users }, ctx) {
  for (const user of users) {
    await createUser(user, ctx);
  }
}

2. this Method

Another way is to take advantage of this in Javascript. This allows accessing ctx via this instead of a function parameter. And so by default this fixes the type issue when calling it from a react component because there's no second argument.

export default async function getUser({ where }: GetUserInput) {
  const ctx = this as GlobalCtx;

  ctx.session.authorize();

  const user = await db.user.findOne({ where });

  return user;
}

And you would chain resolvers like this:

import createUser from "./createUser";

export default async function createUsers({ users }) {
  for (const user of users) {
    await createUser.call(this, user);
  }
}

A downside of this approach is that this input to resolover.call(this, args) is not properly typed. I personally think this is not a big deal because it won't be used that often. And the type isn't a big deal since you are passing this straight through vs manually constructing that ctx value.

3. getSession(this) Method

See below for this new option I added after the initial post.

4. useMutation() Method

See below: #1052 (comment)

Global ctx Type

In both cases, the new way to define types for ctx is via a global declaration. This makes sense because it's for global middleware. And it allows you to reference the type anywhere in the project without importing it.

// types.ts
import { SessionContext } from "blitz";

declare global {
  type GlobalCtx = {
    session: SessionContext;
  };
}

---

So what do you all think? Which approach do you like better? Any better ideas?

[ryardley]

One advantage of option 1 is that it allows for the possability to provide metadata as another argument to formulate an SWR cache key or dependency graph which could possibly abstract away useQuery leading to a simpler out of the box frontend API with managed SWR.

[flybayer]

Yeah definitely. At first I was leaning towards the this option, but I'm increasingly thinking the HOF is the best option.

Also, we could separate resolver() into query() and mutation() if it would help down the road 🤷 .

[flybayer]

Are you thinking the user would provide extra metadata? Or we do it automatically?

Because if we can do it automatically, then we don't need users to add an HOF. We can easily add a HOF with a babel plugin.

[HeyGarrison]

In regards to the second options would this technically work?

export default async function getUser({ where }: GetUserInput) {
  this.session.authorize();

  const user = await db.user.findOne({ where });

  return user;
}

[flybayer]

Yes it would run, but the type of this is any in that case, so you won't have type safety.

[HeyGarrison]

The second options seems way more readable than the first option.

The complexity of wrapping a function in resolver is a little confusing to me. Obviously, as you inspect the function it'll make more sense. The idea of using functionName.call to daisy chain auth was foreign to me. I had to look up the docs because I've never used it before. Of course, now that I know what it is it seems pretty handy...

All of this is from a newb's perspective though so realistically this could fall into the "coaching" category.

[Skn0tt]

I think option 1 could break automatic imports from IDEs.

[flybayer]

Auto import still works fine for me with Vim + TSServer

[SigurdMW]

I think a lot of people try to avoid using this due to the confusion that surrounds the changing nature of it. I would also rather go for a more functional approach that I think is more common in the react community. Also, we should consider consistency - are we going to use this other places in Blitz? (hope not)

[ryardley]

Yeah I think using this is fine if you are using classes without inheritance but using this in a function as an injection mechanism feels like a dangerous precedence when there are other simpler options

[flybayer]

3. getSession(this) Method

This is an improvement on the this method described above.

This uses a helper function to get the actual session object. This results in strong typing without the need to do as GlobalCtx. Also, this getSession() helper function will throw a helpful error if session doesn't exist. Whereas with method 2, you would just get an error about authorize doesn't exist on undefined

import {getSession} from 'blitz'

export default async function getUser({ where }: GetUserInput) {
  const session = getSession(this)
  session.authorize();

  const user = await db.user.findOne({ where });

  return user;
}

And you would chain resolvers like this:

import createUser from "./createUser";

export default async function createUsers({ users }) {
  for (const user of users) {
    await createUser.call(this, user);
  }
}

Optional Syntax Sugaring to Remove this

We could add a babel plugin that would let users do const session = getSession() and the plugin would automatically convert that to getSession(this).

-const session = getSession()
+const session = getSession(this)

This "shields" users from this, but I don't really think it's a good idea. I think it hides too much and therefore will cause extra confusion.

Optional Improvement to Extending res.blitzCtx From Middleware

This would also open the possibility to use WeakMap for extending ctx in middleware which results in much greater runtime safety and type safety. Because with this it's impossible for two middleware's to overwrite each other's data.

Instead of any middleware being able to modify res.blitzCtx, they would each have their own WeakMap to associate their own data to res.blitzCtx. And then each middleware would expose a helper function that would get it's data from res.blitzCtx, exactly like shown just above with getSession().

And actually, if we did this, probably we would get rid of blitzCtx and middleware can just use req or res for its WeakMap.

[HeyGarrison]

Oh. I really like this approach. Would it be a bad idea / unnecessary to have a daisy chain function of some kind? That's the only thing that I find confusing about this approach.

[flybayer]

I think we could remove the .call thing and do that in a babel plugin. But regardless, to test these resolvers, you'll need to do the resolver.call(ctx, args) thing. So I think it's better to be consistent in how they are called directly (from tests or from other resolvers).

[Skn0tt]

I like this one the most. I think .call is a bit of an unknown thing for new JS developers, but that's something we can work around with docs.

[ryardley]

How do we then test resolvers that use getSession()? I assume we would create an object with a session property and pass it to fn.call() Seems we wouldn’t be exposing as much internal baggage if we just went down the road of decorating functions. If we are using this for dependency injection we might as well be using classes and following a repository pattern as those patterns are well understood by folks from OO paradigms.

[ryardley]

Also it is probably a bit of a smell that the session is available in what is effectively a data repository. I have been slowly working up a short essay on the subject but I think we need an isomorphic business logic layer that sits above our current mutations/queries.

[ryardley]

Lastly I because I am dubious about using this as a binding mechanism I am also dubious about doing stuff as complex as Babel transforms to make it more beautiful because in my head we will probably want to throw it all out in the end.

[gabrielcolson]

In my opinion, all of these proposals add a lot of complexity/magic to the "backend" part. I am very pleased with the look the resolvers have right now, even if they could be more strongly typed:

// app/mutations/updateUser.ts

import { Context } from "blitz"
import { User } from "db"
import { UpdateUserInputType } from "../validations"

export default async function updateUser(input: UpdateUserInputType, ctx: Context): Promise<User> => {
  ctx.session.authorize()
  // ...
}

I like to have my types at the right of the = but we could also provide a Query and Mutation types for those who prefer them at the left:

// app/mutations/updateUser.ts

import { Mutation } from "blitz"
import { User } from "db"
import { UpdateUserInputType } from "../validations"

const updateUser: Mutation<UpdateUserInputType, User> = (input, ctx) => {
  ctx.session.authorize()
  // ...
}

I think we should do everything we can to preserve this simplicity and strong typing.

My proposal: useMutation method

Move this complexity to the frontend as it is already done for the queries.

Possible usage

// app/pages/index.tsx

import { useMutation } from "blitz"
import updateUser from "app/mutations/updateUser"

const HomePage = (): JSX.Element => {
  const [updateUserMutation] = useMutation(updateUser)
  return (
    <button onClick={() => updateUserMutation({name: "toto" })}>
      Call me toto
    <button />
  )
}

export default HomePage

Types

Here are the types I came up with:

import { SessionContext } from "@blitzjs/core"

export interface Context {
  session: SessionContext
}

type Mutation<Input, Result> = (input: Input, ctx: Context) => Result

declare function useMutation<Input, Result>(
  mutation: Mutation<Input, Result>
): (input: Input) => Result

Avantages

This hook has several benefits:

• it keeps the backend code simple (damn I love it)
• I agree with the fact that calling a mutation just like any other function is very cool but let's face it: they are not like any other function. There is a lot of magic happening under the hood and having the useMutation would be more explicit about the fact that there is magic happening here
• React developers are probably already used to this kind of hooks: ApolloClient, react-query, etc
• Blitz already solves this problem on queries with useQuery and ssrQuery. Having a useMutation hook (and a ssrMutation function even if I don't think it will be used a lot) would be more consistent with the rest of the API.

[flybayer]

Thank you so much for taking the time to write this up!

This is a really solid proposal! I also really like having the server code so clean, hence why I don't really like the HOF method. And you're definitely right that the magic/complexity is on the frontend but the previous proposals are trying to compensate for that on the backend. And you have a good argument that we should compensate for that on the frontend, where the complexity actually is.

The only thing I can think of that would also need changed, is how we prefetch queries. Currently to prefetch you do await getUser({where: {id:1}}). But that would have to change because of the types.

Here's one option for that:

import {prefetchQuery} from 'blitz'

await prefetchQuery(getUser, {where: {id:1}})

We could also provide a non-hook function that would allow calling a query or mutation directly (same end result as we have currently).

import {execute} from 'blitz'

const result = await execute(getUser, {where: /*...*/})
const result = await execute(updateUser, {data: /*...*/})

[flybayer]

This also brings us more in-line with react-query. Probably we can and would simply wrap the react-query useMutation hook which gives us features like mutation isLoading so you don't have to track that yourself like you currently do.

const [updateUserMutation, {isLoading}] = useMutation(updateUser)

[flybayer]

@ryardley what do you think about this proposal?

[ryardley]

I think it is the best solution so far.

[flybayer]

Yeah same. Unless we get some other feedback within the next few days, I'll go ahead and add the useMutation hook.

[flybayer]

Question from Slack:

So this proposal would consist of wrapping a mutation in a useMutation query which would take care of passing through context and it would be strongly typed? So then if you don’t wrap a mutation with useMutation then context wouldn’t be passed? Am I missing something?

ctx is always passed by the blitz server internals, regardless of what you do on the frontend. So the core issue is a type mismatch when calling the resolver function directly. Because on the backend ctx is there, but on the frontend you can't pass ctx.

So useMutation will "clean up" the type signature and hide ctx from you on the client, because it doesn't actually exist on the client. And then also provide new features like isLoading.

Does that help?

[HeyGarrison]

So based on this approach if you accidentally called the resolver function directly without useMutation it would still work, right?

Because fundamentally the issue is a type issue?

[flybayer]

Yes. But probably we'd change the implementation to prevent that.

[BjoernRave]

Not sure if this is a good idea, but graphql code generator is generating a hook for each mutation and query to be used directly, so you only need to import one thing and you easily get all the types:

https://graphql-code-generator.com/docs/plugins/typescript-react-apollo#usage-example (at the bottom)

Maybe a similar thing could be done for blitzjs

[flybayer]

@BjoernRave we currently don't have a code generation step (unlike graphql), and we really want to keep it this way!

[BjoernRave]

okay, fair enough. May I ask why you are trying to avoid code generation?

[flybayer]

Because it's a huge headache and bad DX :)