[rust] Better support for abstract types (unions and interfaces) #3280
Conversation
…t relay-compiler-language-typescript package output.
…unions and intersections in the output.
…ture proof abstract types and future proof enums.
There was a problem hiding this comment.
@kassens has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Left some comments on parts. There's a bit work left here to make it possible to merge. We have a huge existing codebase, so this affects a lot of generated files internally that break them. We might have to make the advanced code generation behind config options.
I'm unsure about the combinational explosion in the test outputs. What do you think of generating the vast majority of the tests only in 1 configuration option and then having a few tests that test the differences the options produce? A way I can think of to do that would be either to create additional fixture folders or a thing we did in other places is adding a "header" with options to the file, i.e.
{
futureProofTypename: false
}
%%%
query Test {
# ...
}
then in the fixture transform function, if the file contains %%% split on that and use
let typegen_options = serde_json::from_string(header);
``
to parse the header as json.
(if you have questions for any of this, feel free to reach out!)
compiler/crates/relay-typegen/tests/generate_flow/fixtures/typename-on-union.expected
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback @kassens! Was a bit busy lately so didn't get around earlier to make changes based on your comments. Please take a look when you have time, and happy holidays :) |
This happened when there was a combination between a concrete type with a selection that overlapped with selections on an interface, that is also implemented by the concrete type. In such cases, the base selections are added to the selections of the concrete type, but that could result in duplicates. Now the selections are merged instead.
For example, allow excluding `Node`, to avoid super long lists of concrete types.
|
Thanks for getting back to me @alunyov! I've moved to a different apartment the other weekend, so didn't have much time in the past weeks to dig deeper into where the code stands. Looking at the old discussion over at #3178, I think the feedback there can be summarized as follows:
Have I missed anything? Regarding your own points, I have dug into the issue with the duplicated If a rollout plan has been discussed, I am not aware of it. |
|
Thanks @MaartenStaa I'll try to import this for internal review. |
|
@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Hi @MaartenStaa — thanks for the awesome work on this diff! I'm really excited about this. This will improve the ergonomics of some work that I'm doing. I'm sorry that I didn't review this sooner, and have different feedback than @kassens et al. First, the diff changes the order of generated fields and unions. This is going to make this very hard to land internally. Can you figure out a way to keep the order the same, or else could you split this into two PRs, one of which alphabetizes the order of fields, unions, etc., and the other which is this PR? Alternatively, I could work on alphabetizing fields internally, since I am planning on making that change myself, anyway. Second, I think this shouldn't be set by global configuration. Let's look for the presence of a schema directive on the type/enum to determine whether to make the type/enum future safe. A sketch of how this might work is:
How does that sound? Once we agree on the contours of the diff, I can review the implementation in more detail. |
|
Okay, having thought about this diff some more, the typegen config is a good place to put a global default for the future-proofed-ness of these enums. If there is a default, the type-level directives should be local overrides, in which maybe a good directive + parameter would be Anyway, I'll go ahead and review the implementation of this diff! |
|
Thanks @rbalicki2! Curious to hear what you think of the implementation. I'll try to look into the field ordering in the coming days. So for the Looking forward to hearing your thoughts. I just want to make sure I understand which direction you'd like to go in before starting on an implementation. |
|
I'm envisioning a global future-proofness setting, set e.g. in the typegen config. Then, developers would override that setting for individual types in their schema file. Yeah, you're right — client only types should not be future proofed, ideally. |
|
Hi @MaartenStaa , I started reviewing your diff. This is a lot! Thank you so much for taking the plunge. I'm sorry that you're dealing with relay-typegen/src/lib, and in particular, with the Anyway, so, because it's so hard to reason about this file, I think it'd be best if we paired so that I could understand exactly what's going on in your changes. Can you DM me on Twitter? @statisticsftw or DM me on the GraphQL discord? That being said, this needs to be broken up into smaller diffs. Here are some ideas for landing this in discrete chunks:
I think if we divide it up like that, it'll be a lot easier to review the meaty final diffs. |
Differential Revision: D34470327 fbshipit-source-id: 561dcf8b8763fb05261a33dbbb5251140f3715ef
…bstract-types This removes the intersection type, so we also simplify the output, even if exact objects aren't supported (i.e. for TypeScript). A generated type of intersection(union(type_a, type_b), shared_props) is now instead generated as union(type_a_plus_shared_props, type_b_plus_shared_props) This also means the output between Flow and TypeScript is now following basically the same structure, with the main difference being that Flow supports exact objects.
Summary:
# This diff stack
* This diff stack will sort the following items in generated graphql types:
* Sorting of Inexact and Exact object keys
* Sorting of fragment names in the right hand side of `$fragmentSpreads: Foo$fragmentType`
* Sorting of union types
* Addition of parentheses around union types
* Sorting these is controlled by a rollout parameter, so we can change all the files in www in parts.
# This diff
* Sorts the props of Exact and InexactObjects (if the config option is enabled, or omitted)
* Sorts props in the following order:
```
enum PropSortOrder {
Typename,
ClientId,
KeyValuePair,
GetterSetterPair,
ObjectSpread,
FragmentSpread,
}
```
* Sort props at the creation time of Exact and Inexact objects, not at write time
* Why sort things? There are a variety of situations in which the ordering of fields in the emitted output is arbitrary. For example, the order of fields in a linked field with inline fragments that gets merged into a linked field with optional keys is arbitrary. A developer might refactor relay-typegen to be more efficient but in the process change that arbitrary order. This change will make that refactor easier to land.
* Within a Prop, sort by the ASTStringKey sort order of the prop name (either lefthand side or the spread value).
* Also, derive Ord, Eq, PartialOrd and PartialEq for AST and related items.
# Files to review
* flow.rs and typescript.rs -> the only changes are to tests, you can safely ignore these files
* lib.rs -> uses the new APIs. Mostly, this means passing `self.should_sort_typegen_items` whenever an `InexactObject` or `ExactObject` is created
* writer.rs -> impl Ord in a custom way for Prop, sort props when creating InexactObjects and ExactObjects
* other files -> generated changes
cc kassens
----
Trying to start split #3280 into smaller parts. When writing objects (whether exact or inexact) in typegen, sort the object's members by name. The used sorting is a bit subjective, so I'm open to suggestions.
rbalicki2
Pull Request resolved: #3808
Test Plan:
Changes to generated files for the test project and fixtures.
See D34469747 for changes this would do to generated files in xplat with rollout: 100. TLDR works as expected.
Reviewed By: mofeiZ
Differential Revision: D34439437
Pulled By: rbalicki2
fbshipit-source-id: a0c3825dc6259cb047d876e22c4ac8fcbd01eccc
This is the Rust equivalent of #3178. What I have added compared to that is I made the future proofness of enums and abstract types optional and configurable.
Since the feedback there was positive but the main objection was that the old Relay compiler is basically in support mode until the Rust version is released, I ported this.
Please let me know what you think! I'm personally not that fond of the current setup I have with testing different combinations of future proof enums/abstract types and how this is captured in the fixtures, maybe someone else has a better idea?
/cc @kassens @josephsavona @tyao1