Statically typing cells?

[corbt]

Hey everyone!

I had a proposal on statically typing cells, and would love feedback. I know there are a lot of tradeoffs involved and other folks have been thinking about this!

Essentially, my proposal involves making withCellHOC part of the public API and letting users explicitly call it to create cell components. There would need to be a few modifications to withCellHOC for full typing support -- most importantly, it should be possible to tell it which props the cell expects. A basic sketch of the updated type signature would look something like this. (Note that I've renamed withCellHOC to createCell since I think that's a clearer name.)

type CreateCellArgs<CellProps> = {
  QUERY: DocumentNode | ((variables: Record<string, unknown>) => DocumentNode)
  Success: React.FC<CellProps & Omit<OperationResult, "error" | "loading" | "data">>;
  Failure?: React.Component<CellProps & Omit<OperationResult, "error" | "loading" | "data">>;
  // ...etc
};

export default function createCell<CellProps>(args: CreateCellArgs<CellProps>): React.FC {
  // current contents of withCellHOC go here
}

Consider the following cell, adapted from the tutorial:

export const QUERY = gql`
  query CommentsQuery($postId: Int!) {
    comments(postId: $postId) {
    // ...
    }
  }
`

export const Loading = () => <div>Loading...</div>
export const Empty = () => <div>No comments yet</div>
export const Failure = ({ error }) => <div>Error: {error.message}</div>

export const Success = ({ comments }) =>
  comments.map((comment) => <Comment key={comment.id} comment={comment} />)

The same cell using createCell would be defined as follows:

const QUERY = gql`
  query CommentsQuery($postId: Int!) {
    comments(postId: $postId) {
    // ...
    }
  }
`

const Loading = () => <div>Loading...</div>
const Empty = () => <div>No comments yet</div>
const Failure = ({ error }) => <div>Error: {error.message}</div>

const Success = ({ comments }) =>
  comments.map((comment) => <Comment key={comment.id} comment={comment} />)

// New line here:
export default withCell<{ postId: int }>({ QUERY, Success, Failure, Empty, Loading })

This does add one line of code, of course. But in return, cells and their props can be strongly typed, which is a big win!

This also doesn't have to be mutually exclusive with the existing export const ... syntax. The babel transform could just check whether there's a default export, and skip applying itself in that case.

[corbt]

@thedavidprice mind tagging the relevant team members? I know you mentioned several people have been thinking about this but I didn't catch their names.

[thedavidprice]

looping in @peterp @dac09 @Tobbe

Also, @Krisztiaan might be interested as well

Thanks all!

[dac09]

This is a great suggestion, and definitely helps clear up the idea in my head. Using your idea of generics in withCell, What if we did something like this?

// We type the QUERY here!
// Look below
const QUERY: Document<{comments: string[]}> = gql`
  query CommentsQuery($postId: Int!) {
    comments(postId: $postId) {
    // ...
    }
  }
`

const Loading = () => <div>Loading...</div>
const Empty = () => <div>No comments yet</div>
const Failure = ({ error }) => <div>Error: {error.message}</div>

const Success = ({ comments }) =>
  comments.map((comment) => <Comment key={comment.id} comment={comment} />)

And when when we process the file with the babel plugin we add the new line automatically, but slightly differently.

// Generated code ->

// Something to dig out the type from inside Document
type GqlReturnType = xxx
// This is pseudo type code, have to look up the syntax for "if afterQuery exists"
type GqlReturnOrAfterQuery = ReturnType<afterQuery> || GqlReturnType<Query>

export default withCell<GqlReturnOrAfterQuery>{ QUERY, Success, Failure, Empty, Loading })

The key is that the type detection is handled automatically, with no need to add the extra line. Just throwing my idea out there, not sure whether its a solid plan, happy to hear otherwise.

---

Also: what this might let us do is use type generators https://www.graphql-code-generator.com/docs/plugins/typescript (not tried it yet).

[corbt]

@dac09 if the export default ... line is added in the babel plugin, will the Typescript typechecker know it's there? My understanding was that Typescript operates on the user's source file directly, not on the babel-generated output. That's why I thought the line needed to be added to the source itself.

[dac09]

Typescript operates on the user's source file directly, not on the babel-generated output

You're right... this is why you don't try to figure out solutions late at night. I'll have a think tomorrow!

[cjreimer]

I love the iteration of ideas!
I threw out an idea in #2140 that has some similarities to what @corbt is proposing, under a slightly different context that could also address typescript issues. Happy to hear any feedback ... I'm pretty new here.

[peterp]

I really like this proposal. I think it's a solid way forward for our Typescript story. Removes a bit of magic, at the cost of a single line, and remains backwards compatible (until we deprecate the babel plugin). I think we should go for it!

[cjreimer]

@corbt , what are your thoughts on what the generators should produce?

[peterp]

Correct me if I'm wrong, but I think with<Thing> has some historical significance for react and high-order-components? I think withCell is probably my favorite choice, if we don't care about naming semantics, then createCell is grand too.

[Tobbe]

You're definitely not wrong. Even react themselves use that naming convention. See withSubscription here for example

[corbt]

Yeah, withX is definitely a React convention for HOCs but I'm not convinced it's applicable here. Cells arguably aren't even HOCs in the conventional sense. A typical HOC wraps a single child component and enhances it with specific functionality (usually by passing in new props). So withSubscription as a HOC name makes perfect sense semantically -- if I call withSubscription(MyComponent), I'm saying "I want a copy of MyComponent with a subscription passed in."

It doesn't make as much semantic sense with cells though -- if I call withCell({ QUERY, Success }) I'm not saying "I want a copy of QUERY and Success with a cell passed in." Conceptually, withCell is constructing a new component from a number of inputs, not wrapping a single existing component to inject new functionality. That's why I think createCell is a clearer name.

That said, naming is not a hill I want to die on. 😄

[corbt]

@corbt , what are your thoughts on what the generators should produce?

I'd vote to use this syntax for generated TS cells. No opinion on whether to use it for generated JS cells.

[Tobbe]

That said, naming is not a hill I want to die on. 😄

Haha, that's a funny way to put it ⚔️ ⛰️

And I'm fine with createCell too

[peterp]

Totally, a very valid argument, let's go with createCell!

[cjreimer]

@corbt are you taking the lead on the code for this? Let me know if you need any support.

[dac09]

So I thought a little bit more about this with a fresh mind, I think I was conflating two difference aspects of the types:
a) Having types for when the Cell is used (which is what we're discussing here)
b) Having types for the output of the gql query, that can be used in the Success component

and it looks like there's no resistance towards adding a new line (optionally), and I don't really have a strong opinion on this, so happy to continue as is.

[corbt]

b) Having types for the output of the gql query, that can be used in the Success component

This is something really important to me too, but yeah, I think it can be tackled separately. :)

[Krisztiaan]

Thanks for the mention. As a part of #1878 I have also taken steps to add type safety to the Success component (others should already be OK), but reverted it to keep that PR more about the main goal, and because I may have overcomplicated it in the heat of the refactor-moment.

Without any major change, just drilling a generic for the data through the current solution would be enough to secure Success. Another consideration is having "helper" types, eg.: SuccessProps<Data> | LoadingProps | ErrorProps.

I may have not given this thread a thorough enough read, if so, call me out on it, but solving for this problem seems like a quick non-breaking change PR in a couple LOC.

Here's the weird experiment I have stashed from the other PR (not correct, just for entertainment):

[Krisztiaan]

What a beautiful issue number, #2222, an alternative direction to go, with regards to fixing the withCell typing.

[dac09]

Closing this, as implemented in #2208 - thank you @corbt!

More improvements to come on this aspect in the coming releases!