-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[relay] Show a helpful error if a resolver returns an interface with no implementors #4428
Conversation
Awesome! Left a few comments, but this looks spot on so far. |
...relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.graphql
Show resolved
Hide resolved
<generated>:2:35 | ||
1 │ # expected-to-throw | ||
2 │ query relayResolverEdgeToInterfaceWithNoImplementorsQuery { | ||
│ ^^^^^^^^^^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This underline being in the wrong place is not a problem with your diff. Our test util is not fully roust in this case and does not print location info correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it should be underlining resolver_field
here. Just wondering, where does that get determined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here:
.map_err(|diagnostics| diagnostics_to_sorted_string(fixture.content, &diagnostics))?; |
The problem is that the fixture file actually models two different files, and so the error location should actually get mapped to an offset based on if its an error in the schema portion of the file or the fragment portion.
...pile_relay_artifacts/fixtures/relay-resolver-edge-to-interface-with-no-implementors.expected
Outdated
Show resolved
Hide resolved
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@monicatang merged this pull request in 5d22d1c. |
Context:
Without this we generate a broken Flow type.
We will eventually support Relay Resolvers that return interfaces. Today they are not permitted in the common case. However, once we do allow them, we must ensure that the schema includes at least once concrete type that implements that interface. Without that property, we will generate invalid Flow types.
This change
Adds additional validation for implementors of client-defined interfaces if they are being used as a return type for RelayResolvers and tests. Also updates an existing test to have it only test the existing validation for blocking interfaces as a return type.
Test plan
./scripts/update-fixtures.sh
cargo test
specifically:
cargo test -p relay-transforms --test client_edges_test
cargo test -p relay-compiler --test relay_compiler_compile_relay_artifacts_test relay_resolver_edge_to_interface_with_no_implementors
cargo test -p relay-compiler --test relay_compiler_compile_relay_artifacts_test relay_resolver_edge_to_interface_with_child_interface_and_no_implementors