Express-graphql: arg parameters in resolve don't inherit from Object

[pszabop]

Given a resolve function that looks like

resolve: (root, args, request) => {
  test =  { uuid: '1234', description: 'lead singer of the Doors' };
  console.log('result: ' + Object.prototype.hasOwnProperty.call(args.person, 'description'));
  // XXX This fails!:  console.log('result: ' + args.person.hasOwnProperty('description'));
  console.log('result: ' + test.hasOwnProperty('description'));
  return db.personUpdate(request.user, args.person);
}

If I try to use args.person.hasOwnProperty('description') I get the error hasOwnProperty is not a function. I'm forced to use Object.prototype.hasOwnProperty.call per this issue in another module

Note that if I create an object similar to args.person I don't get an error. See the test example in the code snippet.

I'm not enough of a javascript guru to say why this is happening, but it is a gotcha for when I"m attempting an update, where I merge the update with the existing object depending on what attributes are set in the update. My unit tests for the database pass (e.g. just like the test object above), but then fails when I attempt have graphql-express use my database.

I've checked, and there is no attribute 'hasOwnProperty' on args.person, per the Mozilla description of Object.hasOwnProperty

[wincent]

This is happening because we use Object.create(null) to initialize the args object, and have done since https://github.com/graphql/graphql-js/commit/b93f0490836c75a83e8ae680545756bbcbb6c2bd.

I think the rationale is that step 1 of CoerceArgumentValues in the GraphQL spec is :

Let coercedValues be an empty unordered Map.

And the closest thing we have in JS to an "empty" object is the kind of prototype-less shell that you create with Object.create(null).

This in turn means that you can probably replace your args.person.hasOwnProperty('description') check with args.person.description !== null or similar and have almost equivalent semantics:

• args.person.description === null would be true when the property is explicitly null.
• args.person.description === void 0 or === undefined would be true when the property is unset (or diabolically set explicitly to undefined).
• args.person.description == null would be true when either of the above is true.
• args.person.description would be truthy for most other values (0 and '' being counterexamples).

I'm going to close this for now, but please comment again if you have any follow-up.

[nfantone]

@wincent This is a problem when you pass your coerced objects around as inputs to other libraries/tools that you _don't control_.

This happened to me today when using dynamo-data-types to convert GraphQL inputs into DynamoDB representations. The module uses obj.hasOwnProperty, which raised an exception. This can happen any time, any where from sources that are out of reach for users. Having to wrap my inputs with ad-hoc objects, like { ...myGraphQLInput }, feels hackish and unsafe.

What's the real benefit around stripping objects from their prototypes?

[IvanGoncharov]

@nfantone It's general problem of JS you can't use standard objects as maps. For example, if you create an argument called hasOwnProperty, toValueOf, toString or with any other property name derived from Object. What will happen to dynamo-data-types if it gets object where hasOwnProperty is not a function?

The module uses obj.hasOwnProperty, which raised an exception.

It should be fixed in dynamo-data-types either by using Object.keys or Object.hasOwnProperty.call(obj, key).

[nfantone]

@IvanGoncharov I'd argue that's not a realistic expectation. "Fixing" all libraries out there is not viable. Not to mention, many library authors could understandably take no action and consider this no be a very valid, correct usage of a native function.

And my original question still stands: besides debatable semantics, what's the benefit in using Object.create(null) for resolving objects? If I wanted to create an object with properties named after attributes of Object.prototype, I'd do it like you do with any other property declaration:

> var o = { toString: 3 }
> o.toString
3

What will happen to dynamo-data-types if it gets object where hasOwnProperty is not a function?

_Exactly my point_. That was actually my question to you. JS is incredibly permissive in what it allows users to do. I could be sending arrays to libraries where the property 0 was made configurable: false just for the kicks - but it would probably be a very bad idea. This is the kind of things we should keep to a minimum or avoid entirely, in some cases.

There's a tacit consensus that array[0] = 2 assigns the value 2 to the start of array, just as there is about the existence of very well known, expected attributes on an Object.

[IvanGoncharov]

There's a tacit consensus that array[0] = 2 assigns the value 2 to the start of array, just as there is about the existence of very well known, expected attributes on an Object.

@nfantone But it shouldn't affect your schema design you should be able to do this:

type Query {
  foo(toLocaleString: Boolean): String
}

---

Actually, it's a question for graphql-js and it's already discussed here: https://github.com/graphql/graphql-js/pull/504
And I don't think we would change anything since it's a big breaking change because following is valid at the moment:

for (const key in args) {
  //No need for hasOwnProperty
}

[nfantone]

@IvanGoncharov Thanks for pointing me to the appropriate issue! Sad to see that it's closed, though.

I recognize that it rather futile now, but I believe your argument for not using plain Object suffers from lack of common sense vs. technical purity. Removing widely used methods from the most commonly used type in JS is only asking for trouble and generating incompatibilities with pretty much everything else out in the wild.

And I still haven't heard/read any real tradeoffs of this approach.

And I don't think we would change anything since it's a big breaking change because following is valid at the moment:

This is a rehash of the argument in your original response. To me, this reads dangerously close to: "It's not us doing it wrong, _it's everyone else_".