-
Notifications
You must be signed in to change notification settings - Fork 50
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
[Discussion] Enums/Unions and exhaustiveness #463
Comments
First; I agree this should be togglable. I think enums might already be possible to toggle globally via the Relay compiler and some option whose name I can't remember, but that's for "future proof enums". You can also set the behaviour per field via
Anecdotal, but I've actually worked at multiple places where the main product was a web app, where we'd regularly have a tail of 3+ month old web clients still making requests, because people never close the tab nor refresh the page.
This was a trade off that was made a few years back. This is actually solved in V3 where enums are regular variants with a built-in catch all So to sum it up, I agree, it should be togglable. We'd probably need a new option in the compiler config though. |
Yeah, this is why we check backend version to prevent stale clients from pinging servers. |
Plan on seeing if I can tackle the union part next week. Are you on RescriptRelay v3 already? |
Sorry, my notifications on GH are |
Right, got it. We'll let this rest then. |
As a continuation of the discord discussion regarding enums/unions exhaustiveness.
Docs mention having exhaustive enums and unions is unsafe due to possible gaps between server/client releases. In the case of React Native or web apps with service workers, it makes sense. But in the case of traditional web apps, I would argue the risks are relatively low. And if an app implements version checking (eg, via headers), there is no risk at all.
Also, I have a case when the risk is minimal regardless of the nature of a client. On the backend, I have a macro that generates *Result unions. I'm fairly confident that these unions will never be extended, but still forced to handle
#UnselectedUnionMember
each time I dispatch a Result (quite often).If in the case of Unions,
#UnselectedUnionMember
is an inconvenience. But in the case of enums,_
it might hurt. One of the main reasons we use GraphQL is the ability to sync interfaces between server and client, thanks to the GraphQL type system. I.e., when I extend an enum in the backend schema, I can be confident that after the schema sync, Rescript compiler would poke me in every single place where I need to review my code that depends on the extended enum. But when_
is added, it swallows all new constructors, so it requires manual searching to locate all the places where we depend on this enum to review them.Proposal
Introduce PPX flags
--exhaustive-enums
--exhaustive-unions
that would allow choosing a strategy of handling enums and unions on per application basis.It makes sense to have a local attribute
@exhaustive
that makes unions/enums exhaustive for cases when in general, developers prefer to have a#UnselectedUnionMember
seatbelt, but in some cases, it is not needed (like the example with generated Result unions).What do you think?
The text was updated successfully, but these errors were encountered: