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

Allow null, undefined, NaN as _value_ (NOT _keys_) for Enum type #832

Closed
nodkz opened this issue Apr 28, 2017 · 2 comments
Closed

Allow null, undefined, NaN as _value_ (NOT _keys_) for Enum type #832

nodkz opened this issue Apr 28, 2017 · 2 comments

Comments

@nodkz
Copy link
Contributor

nodkz commented Apr 28, 2017

According to last changes in #812 Forbid 'true', 'false' and 'null' as key names for Enum due spec:

EnumValue : Name but not true, false or null

I got some problems with graphql-compose-elasticsearch (GraphQL wrapper for Elastic API) where some arguments are defined as GraphQLEnums and can contain booleans. So the problem was resolved by providing for boolean values specific keys boolean_true and boolean_false:

function generateEnum(typeName, vals) {
      const values = vals.reduce(
        (result, val) => {
          if (val === '') {
            result.EMPTY_STRING = { value: '' };
+          } else if (val === 'true' || val === true) {
+            result.BOOLEAN_TRUE = { value: true };
+          } else if (val === 'false' || val === false) {
+            result.BOOLEAN_FALSE = { value: false };
+          } else if (val === 'null' || val === null) {
+            result.NULL_VALUE = { value: null };
          } else if (Number.isFinite(val)) {
            result[`NUMBER_${val}`] = { value: val };
          } else {
            result[val] = { value: val };
          }
          return result;
        },
        {}
      );

      return new GraphQLEnumType({
        name: typeName,
        values,
      });
}

It works something like this:
> generateEnum('ComplexEnum', [1, 2, 3, true, false]); 
< enum ComplexEnum {
<   NUMBER_1
<   NUMBER_2
<   NUMBER_3
<   BOOLEAN_TRUE
<   BOOLEAN_FALSE
< }

But this solution does not work for null. Also, it will not work for undefined and NaN as values for enum, cause internally used isNullish method type/definition.js#L979:

function defineEnumValues(type, valueMap) {
    ...
    return {
      name: valueName,
      value: isNullish(value.value) ? valueName : value.value,
    };
}

So if I provide for GraphQLEnum such values

{
  BOOLEAN_FALSE: { value: false },  // will return `false` in resolve method
  NULL_VALUE: { value: null },      // will return `NULL_VALUE` in resolve method instead of `null`
}

I suppose that there should be another check in the helper method defineEnumValues, which should use valueName as value only if property/key value.value is not defined in object.

      value: value.hasOwnProperty('value') ? value.value : valueName

I did not found any restriction in spec that Enum values (not its keys) can not be false, null, undefined or NaN.

Or maybe I something missed?

@leebyron if i'm right, then will be nice to allow nullish values in 0.9.5 ASAP (cause v0.9.4 brings for me some breaking changes with ENUMs, and my suggested changes can also break someone's code). Better to introduce breaking changes for ENUM in one pass, cause not so many devs have yet updated to 0.9.4.

@leebyron
Copy link
Contributor

Yeah - let's not use isNullish for checking enum enum value definitions.

leebyron added a commit that referenced this issue Apr 29, 2017
@leebyron
Copy link
Contributor

btw, names are case sensitive, so you could name them NULL or FALSE and not encounter the name collision

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

2 participants