Issue when mutating fields in Directive

[bradennapier]

So continuing to play with directives, one thing I realized was when trying to do something similar to (actually this direct example breaks as well):

class UniqueIdDirective extends SchemaDirectiveVisitor {
  visitObject(type) {
    const { name, from } = this.args;
    const fields = type.getFields();
    if (name in fields) {
      throw new Error(`Conflicting field name ${name}`);
    }
    fields[name] = {
      name,
      type: GraphQLID,
      description: 'Unique ID',
      args: [],
      resolve(object) {
        const hash = createHash("sha1");
        hash.update(type.name);
        from.forEach(fieldName => {
          hash.update(String(object[fieldName]));
        });
        return hash.digest("hex");
      }
    };
  }
}

We would end up with a problem, the schema will never load in the Playground. Now mine was a bit unique from this as I am implementing a @rename(to: "") to fields (again just playing and learning, not actually putting these things into production).

In order to fix it, it would appear something along these lines is required:

visitObject(type) {
    const { name = 'uid', from } = this.args;
    const fields = type.getFields();
    if (name in fields) {
      throw new Error(`Conflicting field name ${name}`);
    }
    fields[name] = Object.assign(Object.create(fields[Object.keys(fields)[0]]), {
      name,
      type: GraphQLID,
      description: 'Unique ID',
      args: [],
      resolve(object) {
        return 'uid';
      },
    });
  }

Although I am not sure if that would be causing some other major issues.

Another issue in my case is that I want to remove a field all together but that appears impossible - no matter what I do it remains in the schema that is shown on playground!

I did a quick inspect in the browser console and captured this as the error, hopefully it is helpful!

Uncaught Error: {
  "errors": [
    {
      "message": "Cannot return null for non-nullable field __Field.isDeprecated.",
      "locations": [
        {
          "line": 39,
          "column": 5
        }
      ],
      "path": [
        "__schema",
        "types",
        2,

[bradennapier]

Note that this is resolved by adding isDeprecated: boolean to the value.

Also - not sure if it is defined anywhere but I had a hell of a time with rename until I read source and realized i can just return a new field and it will replace the field.

Note: This also doesn't work as other parts of the schema will not update properly and it makes it impossible to use the new field.

I personally think it would make more sense to provide helper functions that allow manipulating things such as fields.

helper.removeField(name: string)
helper.insertField(name: string, field: Object)
// ...

This is a bit easier to define than "return null to delete, return a field to replace"

Luckily this is easy to do with the second argument being sent as an object with a single key atm.

Note that it might be interesting if the types were implemented as a Map or Set as then iterating would make it easier for the caller to add values:

// A more powerful version of each that has the ability to replace or remove
// array or object keys.
function updateEachKey<V>(
  arrayOrObject: IndexedObject<V>,
  // The callback can return nothing to leave the key untouched, null to remove
  // the key from the array or object, or a non-null V to replace the value.
  callback: (value: V, key: string) => V | void,
) {
  let deletedCount = 0;

  Object.keys(arrayOrObject).forEach(key => {
    const result = callback(arrayOrObject[key], key);

    if (typeof result === 'undefined') {
      return;
    }

    if (result === null) {
      delete arrayOrObject[key];
      deletedCount++;
      return;
    }

    arrayOrObject[key] = result;
  });

  if (deletedCount > 0 && Array.isArray(arrayOrObject)) {
    // Remove any holes from the array due to deleted elements.
    arrayOrObject.splice(0).forEach(elem => {
      arrayOrObject.push(elem);
    });
  }
}

This could essentially become:

function updateEachKey(
  mapOrSet,
  callback
) {
  mapOrSet.forEach((value, key) => callback(value, key, mapOrSet));
}

Since:

1. If the called fn adds a new value to the map or set, it will be iterated without any changes, a feature unique to collections over objects and arrays.
2. No needs to worry about the pope (holiness) :-P
3. Easy to convert later.

I am not familiar with all the places that is used of course. Also it wouldn't be unheard of that someone does something and returns a value without considering it would cause an issue here.

[yaacovCR]

In reference to original issue, you need to call new GraphQLObjectType(...) rather than Object.assign(Object.create(..), ...)

In terms of a functional schemaDirective pattern, see #1306 which will hopefully close #1234 by release of v5