instanceOf: disable dev instanceOf checks if NODE_ENV not set #4188
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
yaacovCR
added
the
PR: breaking change 💥
yaacovCR
force-pushed
the
switch-default
branch
3 times, most recently
from
903f7b6 to
a20454f
Compare
development behavior should be opt in, not opt out graphql#4075 (comment)
yaacovCR
force-pushed
the
switch-default
branch
from
a20454f to
b1daf03
Compare
|
This will still not fix #4017. An alternative to this PR would be to rearchitect the instanceOf checks more completely.... |
|
@yaacovCR yes as proposed in graphql/graphql-js-wg#125 |
src/jsutils/instanceOf.ts
Outdated
| globalThis.process != null && | ||
| // eslint-disable-next-line no-undef | ||
| process.env.NODE_ENV === 'production'; | ||
| process.env.NODE_ENV === 'development'; |
There was a problem hiding this comment.
test and development should both invoke the non-production behavior.
There was a problem hiding this comment.
To minimize the size of this PR, I'd personally rewrite this to just:
const isProduction = process.env.NODE_ENV !== 'development' && process.env.NODE_ENV !== 'test';Then the lower lines in this PR shouldn't be needed.
There was a problem hiding this comment.
so this is now:
const isProduction = globalThis.process == null || (process.env.NODE_ENV !== 'development' && process.env.NODE_ENV !== 'test');retaining the check to see whether process is defined globally
There was a problem hiding this comment.
JoviDeCroock/graphql-minifier-experiments#1
I have not gone through all the bundlers, but I think that flipping the default would require us changing our instructions about what to set globalThis.process to, now we would have to set to null
https://github.com/graphql/graphql-js/blob/16.x.x/website/docs/tutorials/going-to-production.md
and if I remember correctly, the reason that is in at all is to support cloudflare.
So I am not sure if I want to move forward with this PR actually.... as if we have a better long-term solution, we should move to that without this intermediate stage......?
There was a problem hiding this comment.
Long-term solution: 3 versions of this file - one with the development export condition, without any of this logic, one with the production export condition without any of this, and one default that falls back to this (or a similar) solution.
There was a problem hiding this comment.
@yaacovCR What problems are you seeing?
Generally, I'd say that unless we publish only a single file, users will have to adopt import maps, but that's the future of ESM on the web anyways.
PS: see https://github.com/apollographql/apollo-client/blob/main/integration-tests/browser-esm/html/jspm-prepared.html for an example
There was a problem hiding this comment.
(That said, if we were to continue with #4221, we would need to ship a rolled-up browser entry point that didn't reference imports - maybe with a separate development and production build.
To be honest, I don't really think we'd need sub-entrypoint browser builds, people who run without a bundler probably don't care about saving bundlesize anyways.)
There was a problem hiding this comment.
I’m not sure if we want to require the use of an import map but I would definitely defer to @JoviDeCroock on that.
There was a problem hiding this comment.
@phryneas Import maps are still being adopted and imho aren't in a well enough state yet for a reference implementation based on a standard to adopt nor is the spec far along enough for it. My two cents here being that either we remove instanceof all together and replace it with a branded symbol/types only approach or we keep the status quo but default to production instead.
There was a problem hiding this comment.
I don’t like adding a dependency just to set an envvar; but 🤷♂️
It seems possible for globalThis.process to exist without globalThis.process.env, which would cause a throw. But that’s an issue with the existing code anyway.
Looks alright; I can’t comment whether it impacts bundling/tree-shaking.
development behavior should be opt in, not opt out
#4075 (comment)