Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposition for an improvement on how interface is handled #315

Closed
reck753 opened this issue Mar 25, 2022 · 3 comments
Closed

Proposition for an improvement on how interface is handled #315

reck753 opened this issue Mar 25, 2022 · 3 comments

Comments

@reck753
Copy link
Contributor

reck753 commented Mar 25, 2022

Hi!

Not long ago, we moved from rescript-apollo-client to rescript-relay and we are loving it so much!
However, we miss how we worked with interfaces.

For sure you will know how it works here, in rescript-relay, but for the sake of how it works in rescript-apollo-client, I will write down both approaches.

Given:

interface Book {
  title: String!
  author: Author!
}

type Textbook implements Book {
  title: String!
  author: Author!
  courses: [Course!]!
}

type ColoringBook implements Book {
  title: String!
  author: Author!
  colors: [String!]!
}

type Query {
  books: [Book!]!
}

In rescript-relay by trying to get only type specific fields, it would result in polymorphic value:

module BookFragment = %relay(`
  fragment MyComponent_book on Book {
    ... on Textbook {
      title
      courses {
        name
      }
    }
    ... on ColoringBook {
      title
      colors
    }
  }
`)

...

let book = BookFragment.use(book)
// `book` would roughly be:
// type fragment = [
//   | #TextBook({ title: string, courses: array<course> })
//   | #ColoringBook({ title: string, colors: array<string> })
//   | #UnselectedUnionMember(string)
// ]

This is fairly okay, but we would prefer to have title out of type specific inline fragments and to specify it once above:

module BookFragment = %relay(`
  fragment MyComponent_book on Book {
    title
    ... on Textbook {
      courses {
        name
      }
    }
    ... on ColoringBook {
      colors
    }
  }
`)

...

let book = BookFragment.use(book)
// `book` would roughly be:
// type fragment = {
//   title: string,
//   courses: option<array<course>>,
//   colors: option<array<string>>
// }

Right here, we don't like two things, first of which is that we have these type specific fields as optional (which is completely valid of course) while they are defined as mandatory in schema and secondly that we don't have something simple and readable to switch on. Having switch on a tuple of optional values is a valid thing but it can get pretty messy:

switch (courses, colors, something1, something2) {
  | (Some(courses), None, None, None) => <ListOfCourses courses />
  | (None, Some(colors), None, None) => <ListOfColors colors />
  | ...
  | (None, None, None, None) => <Error />
}

This second example, in rescript-apollo-client would return something like this:

type book = {
  title: string,
  fragment: [
    | #Textbook({ courses: array<course> })
    | #ColoringBook({ colors: array<string>})
    | #UnspecifiedFragment(something) // I think `something` was either `string` or some `error` record, can't recall
  ]
}

They basically just add a fragment record field, which is a polymorphic variant. This polymorphic variant is identical to the one we have in the first example.
With this result, we could have something like this, which we would prefer more:

switch book.fragment {
  | #Textbook({ courses }) => <ListOfCourses courses />
  | #ColoringBook({ colors }) => <ListOfColors colors />
  | ...
  | #UnspecifiedFragment => <Error />
}

Again, this is just our preference and opinion on this matter, and a food for thoughts.
As I already mentioned, we recently moved to rescript-relay so, please, let me know if we are missing something out.

Thanks!

@zth
Copy link
Owner

zth commented Mar 25, 2022

Hey, thanks for the question! This comes up from time to time, and I agree fully, I also don't like the current representation and would prefer the alternative you show.

However, these types are coming directly from the Relay compiler, so it's Relay itself that is a bit opinionated here in what types they output. This is an area the Relay team (and contributors) are working on improving (see for example facebook/relay#3280), so with a bit of luck things will improve very soon.

As a work around, what I do is (edit: exactly what you say you're already doing but would prefer not to do 😆 ) I duplicate all selections in the inline fragment spreads. I know this removes most of the interface benefits 😉 So it's not an actual solution. But it's a workaround you can use while we wait for the typegen situation to improve upstream, that I believe will give you the type/runtime representation you're after.

Also - happy you're trying rescript-relay out! Please keep posting issues like this and document any oddities/confusing things you encounter, it's very valuable for improving rescript-relay itself, the docs etc. So keep 'em coming! Also feel free to join our Discord.

@reck753
Copy link
Contributor Author

reck753 commented Mar 25, 2022

Thanks for the quick reply!

I am glad to hear your opinion on this and I am grateful for the explanation! Looking forward these improvements from Relay team.

I will try to contribute to the docs about interfaces!

@zth
Copy link
Owner

zth commented Apr 20, 2022

I'm closing this for now, but I'll make sure to ping you when the Relay team moves forward with underlying improvements on this. Thanks for the docs contributions, really great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants