facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.41k stars 1.83k forks source link

Fragment spread on interface not typed properly in Typescript #4645

Open alex-statsig opened 8 months ago

alex-statsig commented 8 months ago

Ran into a weird issue with interface types + fragments today. It seems like the TypeScript types allow you to pass a fragment that may not match (due to the object being a different type implementing an interface) but will silently break at runtime with nulls that shouldn't be possible. It doesn't even throw an error (which I did see while trying to fix this, when moving SomeFragment_blah inside ... on ConcreteA { ...SomeFragment_blah }).

schema:

type ConcreteA implements BaseType {
  name: String!
}
type ConcreteB implements BaseType {
  name: String!
}
type BaseType {
  name: String!
}

type Query {
  base_type(id: ID!): BaseType!
}

query:

query TestQuery {
  base_type(id: "test") {
    ...SomeFragment_blah
  }
}
fragment SomeFragment_blah on ConcreteA {
  name
}

usage:

...
const data = useLazyLoadQuery(query, {});
return <SomeFragment blah={data.base_type} />

This was problematic for me because I was refactoring some fragments to use a separate query for a dialog, and the behavior in this case changed (originally TestQuery was just a fragment on BaseType which could be used in either a query for ConcreteA or ConcreteB so I think it was deterministic per query which type it was, but now its ambiguous due to the base_type field).

I would expect data.base_type to be conditionally applicable as ConcreteA, perhaps requiring an alias or typename discriminator to practically do this. I was surprised it just silently failed in this case, leading to weird downstream issues for missing fields.

I can followup with a more solid repro / confirming this is true on latest versions, but just wanted to get the issue filed first.

sairion commented 6 months ago

experiencing the same thing. For example, I wanted to add spread for common error types, which implements common Error interface. In your example, I guess type BaseType -> interface BaseType?

I looked into relay-typgen's test case and could not find this case covered

XiNiHa commented 5 months ago

A Compiler Explorer reproduction that also includes some tricky cases: link

alex-statsig commented 4 months ago

Seems like maybe https://relay.dev/docs/next/guides/alias-directive/ will help with this in the future, so likely an expecting shortfall currently