fix for Objects creation

[DxCx]

returned Objects did not have proper protoype.

Closes #484

[leebyron]

Thanks for this - though I have a few notes.

First: many of these should not be changed. Object.create(null) is safer way of using js objects as a hash map, which promises to not conflict with prototype values. Especially anything that's only being used internally should be left as is.

Second: Object.create({}) unnecessarily creates two objects, instead you could just use the simpler {} and only create one object.

[DxCx]

Thanks @leebyron, that's always cool to learn new things :) happy to see this comment!
I'll revert those who are not needed, and fix the object creation.
but let's wait and progress on the issue thread before we merge this..

[DxCx]

After clarifying with lee, the old behavior is the correct one.

[pleerock]

hey @leebyron can you please confirm that it is expected behaviour that object created by graphql is not instanceof Object? (some libs like mine can use instanceof Object to check if its an object to do some manipulations assuming that its an object. Shall such cases be replaced with something else, like typeof?)

[IvanGoncharov]

@pleerock I think it's already answer by @leebyron in this comment: #484 (comment)

The missing prototype isn't a bug, it's an intentional tradeoff. Imagine what would be produced by the query:

query {
  toString: me { name }
  valueOf: someOtherRealField
}

Those aliases would cause those properties to be set on the resulting object. If the resulting object was expected to have ObjectPrototype with toString and valueOf prototype methods, then setting those properties would render them inaccessible - if you rely on them then your code would break. A clever malicious person might try to do this to break your server.

[pleerock]

@IvanGoncharov thanks for the deeper explanation

[epoberezkin]

Mozilla discourages use of null prototypes because of multiple unexpected problems it causes: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

This solution seems worse than the problem it intends to solve (which has multiple other idiomatic solutions - one can either use Map or differentiate between own and inherited properties using available JS functions).

It causes (and will continue to cause) multiple issues in the libraries that reasonably expect objects to be ... instances of Object and not instances of null.

[goloroden]

Couldn‘t agree more. I don’t want to know how many developers before have spent hours to finally find this issue / PR as well. I‘m sorry but defining an obviously problematic behavior as „it’s by design“ seems pretty ... uhm, I don’t want to be disrespectful, but ... ignorant. 😔

[PiDelport]

I just got bitten by this problem too: this causes bizarre comparison failures in tests that are tricky, non-obvious, and often buried deep inside library code that's difficult to inspect and debug.

An option to use ES6 Maps rather than plain objects, as in #2664, might go a long way towards fixing this?

[robross0606]

I also just got bitten by this problem. Not using prototypes on only some types of GraphQL expressions introduces a whole extra dimension of code coverage issues for projects using this project such as Apollo. We cannot necessarily control how a user would issue a query, so we would be at the mercy of the caller as to whether input became a full-fledged Object with prototypes or a dictionary. The current solution here is definitely worse than the problem. At least pick a consistent behavior and stick with it.

[glasser]

As @robross0606 suggests, it's strange that valueFromAST and coerceInputValue make different choices as to whether an input object should have a null prototype or not. (I personally have no opinion about which approach is better, but the inconsistency is surprising.)

[iambumblehead]

graphql should return plain data that can be used safely with other programs. For example, nodejs' native assert will fail when comparing graphql's [Object: null prototype] object with normal javascript objects used by other software,

import assert from 'node:assert/strict';
import gqlreq from './gqlreq.js';

const gqlres = await gqlreq();

console.log(gqlres);
// [Object: null prototype] {
//   user: [Object: null prototype] {
//     id: 'ywj--Y3SxiXk4TEa04Wz5'
//   }
// }

assert.deepEqual(gqlres, {
  user: {
    id: 'ywj--Y3SxiXk4TEa04Wz5'
  }
}); // ERR_ASSERTION, Expected values to be strictly deep-equal

[gothraven]

It was fun reading the comments but did we actually found a way to fix this or do we still think it's actually a feature not an issue ? 😂