Skip to content

Commit

Permalink
Sort object members in typegen (conditionally) (#3808)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Maarten Staa authored and facebook-github-bot committed Mar 11, 2022
1 parent 8beb0cb commit b04b2b7
Show file tree
Hide file tree
Showing 815 changed files with 2,503 additions and 2,402 deletions.
159 changes: 87 additions & 72 deletions compiler/crates/relay-typegen/src/flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,42 +378,46 @@ mod tests {
#[test]
fn exact_object() {
assert_eq!(
print_type(&AST::ExactObject(ExactObject::new(Vec::new()))),
print_type(&AST::ExactObject(ExactObject::new(Vec::new(), true))),
r"{||}".to_string()
);

assert_eq!(
print_type(&AST::ExactObject(ExactObject::new(vec![
Prop::KeyValuePair(KeyValuePairProp {
print_type(&AST::ExactObject(ExactObject::new(
vec![Prop::KeyValuePair(KeyValuePairProp {
key: "single".intern(),
optional: false,
read_only: false,
value: AST::String,
}),
]))),
}),],
true
))),
r"{|
single: string,
|}"
.to_string()
);
assert_eq!(
print_type(&AST::ExactObject(ExactObject::new(vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "foo".intern(),
optional: true,
read_only: false,
value: AST::String,
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "bar".intern(),
optional: false,
read_only: true,
value: AST::Number,
}),
]))),
print_type(&AST::ExactObject(ExactObject::new(
vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "foo".intern(),
optional: true,
read_only: false,
value: AST::String,
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "bar".intern(),
optional: false,
read_only: true,
value: AST::Number,
}),
],
true
))),
r"{|
foo?: string,
+bar: number,
foo?: string,
|}"
.to_string()
);
Expand All @@ -422,39 +426,45 @@ mod tests {
#[test]
fn nested_object() {
assert_eq!(
print_type(&AST::ExactObject(ExactObject::new(vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "foo".intern(),
optional: true,
read_only: false,
value: AST::ExactObject(ExactObject::new(vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "nested_foo".intern(),
optional: true,
read_only: false,
value: AST::String,
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "nested_foo2".intern(),
optional: false,
read_only: true,
value: AST::Number,
}),
])),
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "bar".intern(),
optional: false,
read_only: true,
value: AST::Number,
}),
]))),
print_type(&AST::ExactObject(ExactObject::new(
vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "foo".intern(),
optional: true,
read_only: false,
value: AST::ExactObject(ExactObject::new(
vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "nested_foo".intern(),
optional: true,
read_only: false,
value: AST::String,
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "nested_foo2".intern(),
optional: false,
read_only: true,
value: AST::Number,
}),
],
true
)),
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "bar".intern(),
optional: false,
read_only: true,
value: AST::Number,
}),
],
true
))),
r"{|
+bar: number,
foo?: {|
nested_foo?: string,
+nested_foo2: number,
|},
+bar: number,
|}"
.to_string()
);
Expand All @@ -463,22 +473,23 @@ mod tests {
#[test]
fn inexact_object() {
assert_eq!(
print_type(&AST::InexactObject(InexactObject::new(Vec::new()))),
print_type(&AST::InexactObject(InexactObject::new(Vec::new(), true))),
r"{
...
}"
.to_string()
);

assert_eq!(
print_type(&AST::InexactObject(InexactObject::new(vec![
Prop::KeyValuePair(KeyValuePairProp {
print_type(&AST::InexactObject(InexactObject::new(
vec![Prop::KeyValuePair(KeyValuePairProp {
key: "single".intern(),
optional: false,
read_only: false,
value: AST::String,
}),
]))),
}),],
true
))),
r"{
single: string,
...
Expand All @@ -487,23 +498,26 @@ mod tests {
);

assert_eq!(
print_type(&AST::InexactObject(InexactObject::new(vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "foo".intern(),
optional: false,
read_only: false,
value: AST::String,
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "bar".intern(),
optional: true,
read_only: true,
value: AST::Number,
})
]))),
print_type(&AST::InexactObject(InexactObject::new(
vec![
Prop::KeyValuePair(KeyValuePairProp {
key: "foo".intern(),
optional: false,
read_only: false,
value: AST::String,
}),
Prop::KeyValuePair(KeyValuePairProp {
key: "bar".intern(),
optional: true,
read_only: true,
value: AST::Number,
})
],
true
))),
r"{
foo: string,
+bar?: number,
foo: string,
...
}"
.to_string()
Expand All @@ -513,14 +527,15 @@ mod tests {
#[test]
fn other_comment() {
assert_eq!(
print_type(&AST::ExactObject(ExactObject::new(vec![
Prop::KeyValuePair(KeyValuePairProp {
print_type(&AST::ExactObject(ExactObject::new(
vec![Prop::KeyValuePair(KeyValuePairProp {
key: "with_comment".intern(),
optional: false,
read_only: false,
value: AST::OtherTypename,
}),
]))),
}),],
true
))),
r#"{|
// This will never be '%other', but we need some
// value in case none of the concrete values match.
Expand Down
Loading

0 comments on commit b04b2b7

Please sign in to comment.