Always generate records in the new version of bucklescript where records compile to objects

[jfrolich]

It would be great to have the result of the graphql query to always compile to records in the new bucklescript as there is no conversion overhead anymore.

[ozanmakes]

I don't think performance was a concern when this default was set. Records are nominally typed and they make it harder to write React components that take a subset of fields of a GraphQL node from different queries. I find that especially in early stages of development @bsRecord causes quite a bit of friction.

It would be interesting to have this PPX generate modules & record types for GraphQL nodes though!

[baransu]

I was thinking about it some time ago but in general, it's super hard to make it work that way and is basically 100% rewrite of how ppx works right now. If you have time to experiment, I'm willing to help if you have any questions.

[jfrolich]

Ah ok, nominal vs structural can make quite a difference indeed. Didn't think about accepting data from different queries (is there not another way around with algebraic types?).

Didn't know it was architecturally such a large task, thought it was mainly objects due to the data type compatibility (as we already have @bsrecord). Is generating the types the hard part?

[baransu]

It might not be as big change as I thought. It would require to generate all required flat types (like apollo codegen for flow or typescript - e.g. user_profile_details) and then annotate the result.

[jfrolich]

Ah cool indeed. I might look into it once I finish the other PR. But probably way above my ppx skill level.

[jfrolich]

This will probably make code so much nicer and remove a lot of None checks when we have pattern matching.

[baransu]

Yes but the problem is not implementation itself but rather usage. When you have the same user from two different queries, you won't be able to pass it to a single component. For type system types are different as they come from two different places even if they have the same structure. In this case, objects are better. That's a tradeoff you can solve using @bsRecord as you provide one type for both cases but with generated code it wouldn't be possible.

I'm still battling if it's a good idea or bad to experiment with that. I don't see much value in the record approach for most of the queries.

[MargaretKrutikova]

I would mostly be interested in possibility to update apollo cache without pain (as it is now unfortunately 😞), since everything stored in the cache is plain JS object, and you can't do spread on Js.t and every type for reading-writing to cache in reason-apollo is Js.Json.t, which requires using %identity to convert to and fro the actual types, so it is a real mess 😄

I was hoping that records compiling to JS objects would allow us to use records as generated type from the ppx and therefore allow to read/write to cache the same types? Does it make sense? Maybe this will bring some value to the record approach?

[baransu]

There is #7 and #20 which will help a lot with writing to cache as it will solve the problem of interfaces/unions/enums which are more painful to deal with than object/records.

[jfrolich]

When it comes to the problem with reusability, I think that can be tackled with better fragment support. In short a component defines a fragment of the data it needs, and it always gets the record types of the fragment, that gets rid of the nominal typing issue in most practical scenarios.

[jfrolich]

I started implementing this. However it's not as straightforward as record types are more strict, and basically we have to generate the type of the result before generating the parse function, so quite a big change in the implementation. It's neater though, because now we can have a "raw" type that directly maps to the parsed JSON result with zero runtime overhead, and a parsed type where we convert to the options/variants and other reason data types. This also allows consumers to opt out of the more fancy types and have zero overhead (just types). It also allows to do typed optimistic response or cache updates without needing a serialize function (and still have the spread operator).

[baransu]

Thank you @jfrolich! If you have any questions or need help, please open draft PR and we can work in it here 🙂

I talked with @sgrove and @zth about it and how implementation could look like. Its something we would like to have and this will be the future direction in which graphql_ppx will be moving with default record queries and Js.t as an opt-in (if will be required).

As you pointed there is "raw" potential - I agree but nonetheless I would like to finish #20 anytime soon.

[jfrolich]

Thank you @jfrolich! If you have any questions or need help, please open draft PR and we can work in it here 🙂

I had it almost working, only to find out that the OCaml compiler doesn't allow it (scoping). Once I have a direction that works I will open a PR. If you want to start first that's fine as well, I'll happily join.

I talked with @sgrove and @zth about it and how implementation could look like. Its something we would like to have and this will be the future direction in which graphql_ppx will be moving with default record queries and Js.t as an opt-in (if will be required).

Can you share your envisioned implementation, I think we have to bite the bullet and generate proper types, and thus discarding most of the current code generation code.

Great that you agree this should be default mode. I think so too, and with fragments there are almost no downsides to nominal typing.

As you pointed there is "raw" potential - I agree but nonetheless I would like to finish #20 anytime soon.

Yep! I think with the serialize function it will be even better, just realized even without the serialize function this will be much easier.

[baransu]

Can you share your envisioned implementation, I think we have to bite the bullet and generate proper types, and thus discarding most of the current code generation code.

The idea is to generate type t for the module first. It should be quite easy as it would work the same as parse generation but return types instead of parse functions.
I haven't tested it yet but you probably can pack everything into:

type t = {
  createUser: createUser
} and createUser = {
  friends: array(createUser_friend)
} and createUser_friend = {
  id: string,
  name: string
}

This would allow us to create types and don't care about ordering as and allows types to be recursive. Not sure how it looks from AST point of view but should be doable.

With that parse function should work out of the box or require passing : createUser_friend into result. That would require passing additional path we probably don't have right now (not sure).

The important thing would be generating types for fragments and reusing those types in the main query type t as it would allow users to have a shared type between two different queries. It's important to be able to generate shared components reusing the same subselections.

Another thing would be removing @bsRecord and introducing new @bsAs(t: User.t). It would allow the same mechanism as types from fragments but if the developer had dedicated type, not coming from API layer. Probably good case it someone would like to share code between backend and frontend for example.

And about "raw". I would like to skip that in the initial implementation.

---

The idea is to introduce record mode as soon as possible (not sure if by global or local feature flag). If everything would work, and users would be happy I would like to make it default in 1.0.0 release and start using proper semver. If no one would be interested in Js.t module, we could deprecated it and remove in next semver major release.

What do you think?

[baransu]

@jfrolich 1.0.0 would be also a moment to deprecate BuckleScript 5 and make 6+ a default.

[jfrolich]

@baransu, that is actually 100% of what I had in mind as well (or what I arrived at after a long time :)).

The reason for the "raw" type is that we only need one forced type coercion (the Js.Json.t to Query.t_raw). Then the parse function is a lot nicer as we just need to convert Js.Nullable to option etc. etc. If the raw type is good there is much more safety in these runtime conversions.

[jfrolich]

@jfrolich 1.0.0 would be also a moment to deprecate BuckleScript 5 and make 6+ a default.

I think you mean 7+, actually the Bucklescript docs also are recommending this move ASAP because it also means not having to support OCaml 4.02 anymore.

[baransu]

Yes. Exactly 😅

[davesnx]

When you talk about queries treating all records (like @bsRecord now), I assume that you want to use mutations with variables as records as well, right?

I would be really happy with this! 😄

[jfrolich]

When you talk about queries treating all records (like @bsRecord now), I assume that you want to use mutations with variables as records as well, right?

I would be really happy with this! 😄

Yep!

[jfrolich]

Done

[davesnx]

Sweet thanks!!!