-
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
FIX(Graphql): Add support for input with default values #4540
Conversation
@captbaritone sorry for the ping 🥹 .... is there a way to bump those PRs before they go down the black hole of 2nd page ? if there's something we can do to help ? 🤷 |
Sorry for the delayed response. Can you add a fixture test here? https://github.com/facebook/relay/tree/main/compiler/crates/relay-typegen/tests/generate_typescript/fixtures I think the test schema has an example you can trigger here:
Maybe you could also ensure there'a fixture test that validates what we do for Flow in this case? Info on updating fixture tests can be found here: https://github.com/facebook/relay/blob/main/.github/CONTRIBUTING.md#pull-requests Defaults in GraphQL are confusing, but I believe we should should ensure this generates |
awesome! ❤️ .... i'll take a look at that |
5dddec6
to
9e42fbc
Compare
here... i hope it's better now :) tried to not add input but i prefer avoid touching too much other tests to add this |
id | ||
} | ||
} | ||
} |
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.
Would you mind adding this same fixture to the Flow tests?
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.
arg, my bad... here they are :)
==================================== OUTPUT =================================== | ||
export type FeedbackUnLikeInput = { | ||
feedbackId?: string | null | undefined; | ||
silent?: boolean; |
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.
nice!
9e42fbc
to
d3f7eba
Compare
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@captbaritone merged this pull request in 1a57f08. |
awesome ❤️ thanks for the review |
Hello here 👋
We have a few inputs with default required fields that have default values like here :
but the TS generated for this look like
which is not ideal ...
so i marked the field as optional if it has a default :)