Use no-op for production instanceOf

[yaacovCR]

Re-implements #4386 with a configurable function variable initialized to a no-op function in production builds. This is apparently optimizable by v8, as it has no performance hit when setEnv('development') is not called.

This should also allow for tree-shaking in production builds, as the development instanceOf function should not be included within the production bundle if setEnv is not called, although this has not been confirmed.

To avoid a performance hit, setEnv('development') calls setInstanceOfCheckForEnv('development') which sets the function variable to developmentInstanceOfCheck. Setting a property on globalThis was tested, but reading the property led to a small performance hit.

[yaacovCR]

[yaacovCR]

Tagging people from #4386 @JoviDeCroock @benjie @glasser

[glasser]

Just to note — this is still the same semantics as #4386 where the default is always production?

Honestly I don't really understand the point of doing it this way. It's a really elaborate set of gymnastics to go through just to provide a better error if you've misconfigured things than things silently failing due to instanceof checks being spuriously false ... but you still have to know to turn it on? By what mechanism do we expect people to choose to turn this on?

Is there going to be some sort of tell-tale error message or behavior that, when they Google it, will tell them "oh this might be a multiple-installation issue, run setEnv('development') to see if it is"?

But in that case, couldn't the instructions just say "run npm ls | something to see if you have multiple installs"?

If this was on by default and disabled by process.env.NODE_ENV (ie, we were adding a mechanism to control this feature in environments that don't use process.env.NODE_ENV) I'd get it. But I don't know why anybody would ever think to run setEnvironment('development').

[yaacovCR]

In previous discussions, we basically landed on the fact that our defaults should make sure that devs not informed about our instanceOf checks should default to a fast production rather than a more explanatory development, so the default has to be "no enhanced error message." But we also want to be more cross-platform, avoid build-step requirements, but allow tree-shaking.

More context:

• process.env, globalThis, and typeof process #4075

So that combination means that the error message will be less discoverable, usually reached by getting a particularly puzzling error message and getting eventually to some issues like:

• Getting multiple instances of graphql error despite only having a single version #2801

But actually a more cryptic error message after this change!

But in that case, couldn't the instructions just say "run npm ls | something to see if you have multiple installs"?

As seen in the above issue, it's sometimes difficult to see that there are really two realms or two different versions of graphql because of bundling, and having the additional access point may indeed be somewhat helpful.

Basically, I think we acknowledge the issue of discoverability and reduced utility you raise, but we are forced there based on other constraints. At least, that's where the consensus seems to be. This seems like something we could discuss together at a graphql-js-wg meeting (which have been light on agenda recently).

[benjie]

In Grafast we default to production BUT we output a message if you didn't specifically enable it - i.e. we tell the user that they should choose development or production mode, but we default to production because it's safer from a denial of service point of view (i.e. limits error count, less internal detail in errors, etc etc).

Developers hate these kinds of messages, so it's a strong motivation for them to explicitly choose a suitable environment :)

[yaacovCR]

@benjie that's a helpful workflow!

I added a commit to this PR that allows checking the value of Env (production or development or undefined if not set).

Integrations can then decide to appropriately emit a warning (or error message?) via the appropriate channel if the env was not set, as they might wish to provide this better workflow?

In theory, we could internally within graphql-js check this value and print a one-off error message let's say on graphql(...)/execute(...)/validate(...) calls if env is undefined. I suppose this would just be via console.log(), but we don't do any console logging elsewhere so I am a bit hesitant. It seems like this might be an appropriate first step, and then we can consider adding that later depending on how the ecosystem takes up this change?

[yaacovCR]

To make sure tree shaking works , I think we would need separate environment sets for production and development. And of course we need to test that tree shaking actually still works.