use Symbol.hasInstance for looser instanceof checks

[jquense]

fixes #491

I'm not entirely sure how to field test this but Symbol.for should be resilent over package boundaries since its all the same runtime.

[jquense]

not sure what a better type here would be maybe GraphQLType?

[jquense]

any thoughts? I realize its a sort of silly change but it could potentially solve a bunch of annoying npm/yarn induced issues we see a lot on Gatsby

[felixfbecker]

Why does this have to be "attached"?
MDN has this example:

class MyArray {  
  static [Symbol.hasInstance](instance) {
    return Array.isArray(instance);
  }
}
console.log([] instanceof MyArray); // true

seems more natural to me to define it on the class

[KyleAMathews]

Just to add my vote to this — I hear someone complaining about Gatsby being broken due to the overly strict check every few days now.

[felixfbecker]

Why Symbol.for() and not Symbol()?

[jquense]

one fetches a global symbol by name vs creating a new symbol:

Symbol('foo') == Symbol('foo') // false
Symbol.for('foo') == Symbol.for('foo') // true

[felixfbecker]

I understand, but why is that desirable here? Imo the symbol should be private or if it's public, it should be exported, no retrievable for everyone through a magic string. You just need to save the symbol in a variable and reuse it. Otherwise you might as well just use a regular property.

[jquense]

Its not using a symbol for privacy, it's to avoid muddying up people's namespace on the class for things they can use as property names. With the symbol you don't have to worry (nearly as much) about accidentally overwriting the tag in an instance when you just wanted to define an unrelated property. Incidentally this approach is how React tags elements as "ReactElements"

[bvaughn]

This PR is important.

I hear someone complaining about Gatsby being broken due to the overly strict check every few days now.

This has impacted the reactjs.org repo twice now, most recently with reactjs/react.dev/pull/222. Each time it's cost me time and confusion to debug because the behavior is unexpected and confusing.

[leebyron]

I'm going to close this PR since it doesn't solve the issue at hand, but instead moves it and creates new problems. See my comments in #996 for why this issue is more challenging than this PR's solution.

I would much prefer an approach which first improves the API by introducing predicate functions instead of relying on instanceof, and then leverages those predicate functions for producing much clearer and more actionable error messages.

I'd be more than happy to review PRs which tackled improving these errors

[jquense]

Hmm I don't think I understand the advantage to the predicate function of approach. isn't it essentially going to be the same logic except instead of instanceof checking a class tag, you have a function doing it? I'm not sure otherwise how'd you structure such functions other than doing a bunch of ugly tests for internal properties on the class instances, that hardly seems cleaner than leveraging language features designed for this case?

In any case I probably won't work on a follow-up PR, 4 months of no feedback makes it feel like it'd be a waste of time :/. Thanks again.