graphql-zeus icon indicating copy to clipboard operation
graphql-zeus copied to clipboard

null instead of undefined

Open imbroyury opened this issue 3 years ago • 17 comments

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 https://github.com/graphql-editor/graphql-zeus/issues/171 and https://github.com/graphql-editor/graphql-zeus/issues/184 for v3, it came back and is reproduced in v4.0.4 https://github.com/graphql-editor/graphql-zeus/issues/171#issuecomment-1059293427, likely because of commit https://github.com/graphql-editor/graphql-zeus/commit/a338a8dd14344c0d39deaba67cf17514231347ad.

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

imbroyury avatar Apr 17 '22 07:04 imbroyury

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

aexol avatar Apr 17 '22 10:04 aexol

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.

imbroyury avatar Apr 17 '22 11:04 imbroyury

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

aexol avatar Apr 19 '22 15:04 aexol

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"?

ValentinH avatar Apr 19 '22 19:04 ValentinH

@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

imbroyury avatar May 25 '22 10:05 imbroyury

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

aexol avatar May 26 '22 12:05 aexol

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

imbroyury avatar May 26 '22 13:05 imbroyury

@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

ValentinH avatar May 27 '22 14:05 ValentinH

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

aexol avatar May 28 '22 09:05 aexol

@aexol do you have any updates please?

FilipChalupa avatar Aug 25 '22 12:08 FilipChalupa

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.

GauBen avatar Aug 31 '22 21:08 GauBen

@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 avoids us to use 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)

ValentinH avatar Sep 07 '22 10:09 ValentinH

@aexol any updates on this?

imbroyury avatar May 23 '23 15:05 imbroyury