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

@Arg defaultValue "null" seems not to be recognized #593

Closed
karladler opened this issue Apr 1, 2020 · 9 comments
Closed

@Arg defaultValue "null" seems not to be recognized #593

karladler opened this issue Apr 1, 2020 · 9 comments
Labels
Community 👨‍👧 Something initiated by a community Wontfix ❌ This will not be worked on

Comments

@karladler
Copy link

Describe the bug
Setting the defaultValue like explicitly to null in @Arg('id', { defaultValue: null }) seems not to be applied, when omitting the argument id on request.

To Reproduce

  @Query(() => User)
  user(@Arg('id', { defaultValue: null }) id: string): User {
    console.log('id', id);

    if (id === null) {

Logs id undefined;

Expected behavior
Logs id null; when requested without arguments.

Environment (please complete the following information):

  • OS: OSX
  • Node 12
  • Package version: "type-graphql": "^0.17.6",
  • TypeScript version: "typescript": "^3.8.3"
@MichalLytek
Copy link
Owner

@karladler Can you reproduce the default null value using GraphQL SDL?

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Need More Info 🤷‍♂️ Further information is requested labels Apr 1, 2020
@eezing
Copy link

eezing commented Apr 3, 2020

@MichalLytek Yes, default null is valid in SDL. In addition, providing null as argument in query also produces undefined in type-graphql.

A graphql-js example where null arguments are valid:

https://repl.it/repls/ClearcutRaggedFormat

const { graphql } = require('graphql');
const { makeExecutableSchema } = require('graphql-tools');

const typeDefs = `
  type Query {
    hello(msg: String = null): Boolean
  }
`;

const resolvers = {
  Query: {
    hello: (_, args) => {
      console.log(args); // --> { msg: null }
      return true;
    },
  },
};

const schema = makeExecutableSchema({ typeDefs, resolvers });

const query = `
  query {
    a: hello
    b: hello(msg: null)
  }
`;

graphql(schema, query)

@MichalLytek
Copy link
Owner

MichalLytek commented Apr 3, 2020

And what is the practical difference between:

hello(msg: String = null)

and

hello(msg: String)

🤔

In both cases if argument is not provided, it gonna be null/undefined, and from client perspective the arg is optional.

@eezing
Copy link

eezing commented Apr 3, 2020

Is default null practical? No, not really.

But, an explicit null argument from query does have meaning; for example, delete record if argument is null or leave it untouched if argument is undefined (explicit vs implicit).

@MichalLytek
Copy link
Owner

MichalLytek commented Apr 3, 2020

But there is no such distinction in GraphQL Specification between null and undefined and nothing about the implicit undefined behavior when value not provided and explicit null when null provided.

It's just a case in JS where we have two empty values representation.
So relying on this behavior means your not using GraphQL but GraphQLJS and when somebody change the language of the implementation of this query (other microservice) event if the schema is the same - it won't work anymore 😕

@eezing
Copy link

eezing commented Apr 3, 2020

Ok, I stand corrected. Semantically there is a difference, but like @MichalLytek said, the spec states that we should not treat implicit and explicit null differently.

https://spec.graphql.org/June2018/#sec-Null-Value

@MichalLytek
Copy link
Owner

MichalLytek commented Apr 4, 2020

More context:
graphql/graphql-js#1298 (comment)

So, if query { hello } gives undefined for hello(msg: String) and null for hello(msg: String = null), maybe just treat undefined and null in the same way, like arg ?? value or if (arg != null) 😉

With hello(msg: String = null) you never have undefined but the null value even if somebody doesn't provide any arg value. So you can't "leave it untouched if argument is undefined".

@MichalLytek MichalLytek added Documentation 📖 Issues about docs Discussion 💬 Brainstorm about the idea and removed Documentation 📖 Issues about docs labels Apr 4, 2020
@karladler
Copy link
Author

so { defaultValue: null } is basically doing nothing since it's the same as setting default to undefined.
Since it Maybe there should be a warning generated, bc it's not that logic and if I test later for id !== null the rest of my logic will fail.
A hint in the doc could be also helpful.

@MichalLytek MichalLytek added this to the 1.0.0 release milestone May 2, 2020
@MichalLytek MichalLytek added Documentation 📖 Issues about docs and removed Discussion 💬 Brainstorm about the idea Need More Info 🤷‍♂️ Further information is requested labels May 2, 2020
@MichalLytek
Copy link
Owner

Closing as null as default value is not really practical and supporting this would confuse more people than satisfy, e.g. when they think that defaultValue: null means no default value 🔒

@MichalLytek MichalLytek removed this from the 1.0.0 release milestone May 3, 2020
@MichalLytek MichalLytek added Wontfix ❌ This will not be worked on and removed Documentation 📖 Issues about docs labels May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Wontfix ❌ This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants