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

Bunch of stuff / Proposal #2

Open
dawee opened this issue Apr 3, 2019 · 5 comments
Open

Bunch of stuff / Proposal #2

dawee opened this issue Apr 3, 2019 · 5 comments

Comments

@dawee
Copy link

dawee commented Apr 3, 2019

Hey,

I don't know if it's innapropriate to write all my thoughts as an issue here. After your messages on Discord I really though it was cool that someone comes with an alternative to graphql ppx. After six months of using GraphQL ppx on our project, I have tons of ideas I would like for an automatic parser / serializer between reason types and a graphql schema.

So I thought to start something here as a discussion thread. If you don't like the idea you can close it of course.

My main issue with the state we have with graphql ppx is that we have too much things generated. As we tend to need these types in components, it can be really confusing. You said that if you were to do a PPX, you would probably have to rewrite GraphQL PPX from scratch. I think it would be huge for the Reason community. And I think we would be several people to help you if you started this kind of project.

Let say we have this kind of schema described on the server:

interface FeedItem {
  id: String!
}

type ImageFeedItem extends FeedItem {
  id: String!
  imageUrl: String!
}

type TextFeedItem extends FeedItem {
  id: String!
  text: String!
}

type Query {
  hero(episode: Episode): Character
  droid(id: ID!): Droid
}

I was thinking that we could to what genType is doing to map types on something external:

[@graphqlType "ImageFeedItem"]
type imageFeedItem = {
  id: string,
  imageUrl: string
};

[@graphqlType "TextFeedItem"]
type imageFeedItem = {
  id: string,
  imageUrl: string
};

[@graphqlType "FeedItem"]
type FeedItem = [`ImageFeedItem(imageFeedItem) | `TextFeedItem(textFeedItem)];

So if we're parsing something from a query it would try to find what has been registered with these decorators.

Anyway, I don't know if it makes sense for you. I feel that graphql PPX is less and less maintained and we came in a situation that it's really hard to understand it. Maybe if the type creation was explicit and chosen by the developer it would be easier? I don't know.

@sainthkh
Copy link
Owner

sainthkh commented Apr 3, 2019

1. First of all, I decided to create ppx. Actually, I tried to write why it's impossible this morning and tried to explain why but I was persuaded. You can see how I solved my major concern, fragments, in #3. (If there is no problem, that will be the fragment spec.)

2. It seems that many of us are thinking similar things. Actually, in early stages of ReasonQL, I was thinking about the similar thing. How about creating types from schema and using them in queries?

It is not a problem if we always query every field in every type. But that's not how GraphQL works.

Think about this example.

# Schema
type Game {
  id: String!
  title: String!
  summary: String!
  release: Int!
  explanation: String!
}

type Query {
  game(ids: [String!]!): [Game!]!
}
# Query
query GameQuery($ids: [String!]!) {
  games(ids: $ids) {
    id
    title
    summary
  }
}
/* SchemaTypes.re */
type game = {
  id: string,
  title: string,
  summary: string,
  release: int,
  explanation: string,
};

type queryResult = {
  games: array(game),
};

This query is quite common when we're listing things. But in the lists, we don't need long explanation. In this case, what should we fill with explanation? Just empty string? It's a hard question, isn't it?

To avoid this situation, I also decided to generate types and their codecs for every query. Although most codes are seemingly redundant.

And I believe this might be why this project is almost stopped.

3. But you made me remember one more reason why I decided not to create ppx in the beginning. It generates a lot of things and they're hidden to us. In the new reasonql_ppx, we need to support "printing the generated types and codec on files for reference".

4. Thank you for the interface code. Currently, reasonql doesn't support interface. It's because it's a bit complicated and will take times (I didn't want to think about that) and I thought many people wouldn't use. But your code can be a good starting point.

@baransu
Copy link

baransu commented Apr 3, 2019

I would really like to contribute to Reason GraphQL implementation.

I'm trying to help with graphql_ppx (I've added Interfaces support and other small fixes) but right now I'm using my own fork: https://github.com/baransu/graphql_ppx available as @baransu/graphql_ppx with some fixes that haven't been merged yet.

I have few main problems with graphql_ppx and I think we can fix them:

  • Lack of two way serialization, right now we can decode query result but there is no way to encode query result, only input types which can be helpfull when caching (which is problem right now with WriteQuery and ReadQuery in reason-apollo and poly variant int representation in JS)
  • I don't like objects and poly variants but it's hard to represent query results as variants and records because two types with shared fields cannot be used together in one function/component etc. Maybe I'm wrong. Same goes with variants (but it's less of an issue because enums can be shared for whole project and there is not way to query for subset of enum cases), we have duplicated enum definitions because there is no enum type in graphql_ppx generated module we can rely on
  • Not so strong typed mutations and queries, there are cases when you're not warned about mutation/query arguments that no longer exist in schema
  • No way of handling custom scalars. We're using Sangria on backend (in Scala) and they have Long scalar which is used for example for timestamps representation. I don't like that, GQL doesn't have Long because of JS but we have to handle it somehow. graphql_ppx returns Js.Json.t and we had few cases when someone decoded it by hand to something other than float which is really hard to find.

@dawee
Copy link
Author

dawee commented Apr 3, 2019

@sainthkh

Yes you're right it's a complicated question. I just realize right now my example can't work because you can't predict which fields will be requested.

In our case interfaces is one of our main use. The example I gave you is typical of what we're doing. That's something I'm going to say in my talk in ReasonConf and the example I gave you is the one I use in my slide. We try to use non optional types as much as we can to avoid unecessary optionals in the client. To achieve this, interfaces are often the solution and the example I gave you is simplified version of what we're doing.

@baransu

I really hoped you would join the conversation :). I think that after the project owner you're the one knowing graphql ppx the most.

I'm happy you mention the 2 ways conversion. We talked about it in an issue of graphql ppx I don't know if you remember. That's a big hole in the graphql ppx API right now.

I didn't even know about the possibility of doing custom scalars like long. I think that if we want something that prevent from doing the same mistakes as graphql ppx, we should provide "real world" examples. Then we could search what would be the best way in theory to deal with these examples in the future of the lib.

@sainthkh I know interfaces and possibly the example @baransu gave you are not in your today scope but I think maybe it could be good we take a look how to resolve it today. I don't mean implement it. A PPX code can be a mess very quickly and I think it would be good to see where we want this to go.

Also sorry I said "we" everywhere even if it's your project. @baransu seem to want to join and that's the same for me. If we're talking about doing better than Graphql PPX, it could be a very ambitious project that helps a lot of people to write reason applications in the future.

@thangngoc89
Copy link

thangngoc89 commented Apr 3, 2019

@sainthkh

we need to support "printing the generated types and codec on files for reference".

This is in fact a limitation of ppx in general. That's why Bucklescript and ReasonML doesn't make ppx a smoother experience.
If you use Reason Language Server editor plugin from @-jaredly, there is a feature to expand all ppx tags in the current file.

And I think it would be unfair to not including graphql_ppx's author @mhallin in this conversation. Maybe he can share some insights about the current approach

@sainthkh
Copy link
Owner

sainthkh commented Apr 4, 2019

@baransu

1. I only thought about JSON -> record. But to use Apollo cache, we need the other way. I think we need to add it as an option
2. Currently, I'm thinking about creating [%%graphql_enum] ppx tag. Then, we can generate common Enum types in that file and use it from everywhere. I'm planning to add error decoders there, too.
3. In ReasonQL, arguments are generated as records and encoded to JSON. So, it's not a problem.
4. I'm thinking about that, too. Like timestamp, we need custom scalar for file uploads. We need to find ways to solve that.

@dawee

We all want better GraphQL world for ReasonML developers. Let's try to make it.

@thangngoc89

File generation cannot be the top priority. But it can be useful because:

  1. ppx expansion currently works only in VSCode. (I know VSCode is many developers' favorite. But some people use other editors.)
  2. And sometimes, it's useful to put generated code on one side and write things we need on the other.

I also hope to get @mhallin's insights and ideas. We could be here together thanks to graphql_ppx.

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

4 participants