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

null instead of undefined #262

Closed
imbroyury opened this issue Apr 17, 2022 · 19 comments
Closed

null instead of undefined #262

imbroyury opened this issue Apr 17, 2022 · 19 comments

Comments

@imbroyury
Copy link

Hello @aexol, thanks for your amazing library.

The generated type for nullable fields in v4.0.4 is ... | undefined while in reality the runtime type is ... | null.

The issue was originally reported in #171 and #184 for v3, it came back and is reproduced in v4.0.4 #171 (comment), likely because of commit a338a8d.

Can you give an update on this? Do you plan to replace undefined with null? Perhaps, in v5? If no, why?

@aexol
Copy link
Collaborator

aexol commented Apr 17, 2022

Maybe some day with a flag later. Using nulls make using Zeus extremely painful

@imbroyury
Copy link
Author

Thanks for your prompt reply 🙌

But isn't it ultimately a caveat of using a GraphQL API?
If a server is sending over null for a nullable field as an indication of no value, a developer shouldn't lie to him/herself for convenience.

@aexol
Copy link
Collaborator

aexol commented Apr 19, 2022

They should. It is much easier to manage those objects without nulls. However we can make a config file for zeus to maintain all those flags like nulls vs undefined

@ValentinH
Copy link

We are using Zeus heavily on our codebase with a patch to use null for nullable values. I don't understand what do you mean by "much easier"?

@imbroyury
Copy link
Author

@aexol it seems like it's not just me, so can you please elaborate on the "much easier" part of null vs undefined for other users of the library? 🙂
very interested in your point of view on the problem

@aexol
Copy link
Collaborator

aexol commented May 26, 2022

ok, if you have an object stored and you want to provide values from the frontend side, then every optional field if the type is inferred by Zeus has to be manually typed as null and that doesn't make sense at all. You could have just written types by hand.

https://zeus.graphqleditor.com/page/selector.html

@imbroyury
Copy link
Author

@aexol sorry, I don't think I understand why "every optional field if the type is inferred by Zeus has to be manually typed as null"? can't Zeus infer types as | null?

@ValentinH
Copy link

@aexol for your example, if I need to insert something from the frontend, let's say a user, each field will be optional because ValueTypes is used:
image

Data returned by the API is typed by the GraphQLTypes where the non-required field are typed as null (non-optional):
image

@aexol
Copy link
Collaborator

aexol commented May 28, 2022

I will provide an example when I will have time for that

@FilipChalupa
Copy link

@aexol do you have any updates please?

@GauBen
Copy link
Contributor

GauBen commented Aug 31, 2022

I just ran into an issue because of this. null and undefined do not behave exactly the same:

const {x = 1, y = 2} = {x: undefined, y: null};
console.log(x, y); // 1, null

– A developer shouldn't lie to theirself for convenience.
– They should.

Absolutely not, this feature is breaking type safety.

@ValentinH
Copy link

ValentinH commented Sep 7, 2022

@aexol would you accept a PR where we enable this to be configured via a flag?

I've been maintaining a fork of v4 for a few months and most changes are now useless thanks to the nice changes of v5.
I've only this case + another (I'll create a dedicated issue) that prevents us from using the official version 😅

If this can be useful to someone else, here's the commit on our fork to use null instead of undefined for the returned data ModelTypes/GraphQLTypes: elba-security/graphql-zeus@f298564 (#6)

@imbroyury
Copy link
Author

@aexol any updates on this?

@GauBen
Copy link
Contributor

GauBen commented Jun 27, 2024

@ValentinH Here is a small update to your diff: you need to update MapType to the following

export type MapType<SRC, DST, SCLR extends ScalarDefinition> = SRC extends null
  ? null
  : SRC extends DeepAnify<DST>
    ? IsInterfaced<SRC, DST, SCLR>
    : never;

Otherwise, the Type | null union gets its null smashed by SRC extends DeepAnify<DST>

TypeScript playground

It would be nice to have this fixed directly in Zeus :)

@ValentinH
Copy link

ValentinH commented Jun 27, 2024

Thanks for sharing this.

FWIW we have decided to migrate to https://the-guild.dev/graphql/codegen as the Zeus codegen size (more than 200k LOC as of today) was getting way too big for us (and for Next.js, tsc and VSCode 😅). Also being able to author real Graphql is in the end a win as we don't have to teach yet another tech to the team and we can benefit from more tooling.

@GauBen
Copy link
Contributor

GauBen commented Jun 27, 2024

I was thinking of making the same move for a while now, but with Houdini taking care of the plumbing. Thanks for your tip!

@ValentinH
Copy link

Interesting approach!

@aexol
Copy link
Collaborator

aexol commented Nov 1, 2024

done!!!! Nulls are in responses - also enums are standard not const

@aexol aexol closed this as completed Nov 1, 2024
@GauBen
Copy link
Contributor

GauBen commented Nov 1, 2024

You're awesome @aexol, thank you!

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

No branches or pull requests

5 participants