Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

eMerzh
Copy link
Contributor

@eMerzh eMerzh commented Nov 30, 2023

Hello here 👋

We have a few inputs with default required fields that have default values like here :

input StoryPinInput {
  id: ID!
  isGlobal: Boolean! = false
}

but the TS generated for this look like

export type StoryPinInput = {
  id: string;
  isGlobal: boolean;
};

which is not ideal ...

so i marked the field as optional if it has a default :)

@eMerzh eMerzh changed the title Graphq: Add support for input with default values FIX(Graphql): Add support for input with default values Dec 13, 2023
@eMerzh
Copy link
Contributor Author

eMerzh commented Jan 8, 2024

@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 ? 🤷

@captbaritone
Copy link
Contributor

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:

nameWithDefaultArgs(capitalize: Boolean! = false): String

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 { isGlobal?: boolean } and not { isGlobal: boolean | null | undefined } since I believe with required arguments with defaults you are permitted to omit the argument but not pass an explicit null.

@eMerzh
Copy link
Contributor Author

eMerzh commented Jan 8, 2024

awesome! ❤️ .... i'll take a look at that

@eMerzh
Copy link
Contributor Author

eMerzh commented Jan 8, 2024

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
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@facebook-github-bot
Copy link
Contributor

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@captbaritone merged this pull request in 1a57f08.

@eMerzh
Copy link
Contributor Author

eMerzh commented Jan 10, 2024

awesome ❤️ thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants