Checks for existence of process before process.env for esm build

[lukejacksonn]

Related to #2277 specifically 8e7cc02. The last thing required to make this package run in the browser without bundling.

This change will make instanceOf choose production mode if:

1. The script is being executed in a browser (the global process doesn't exist)
2. The script is being executed in node where NODE_ENV === 'production'

I do not know if there is a more webpack specific check required. That is:

3. Is there be a case where it would be expected to choose production mode of instanceOf but where NODE_ENV !== 'production'.

[IvanGoncharov]

@lukejacksonn Thanks for extracting this PR 👍

I checked how React handles this problem and they use process.env.NODE_ENV === 'production' without testing for process:
https://unpkg.com/browse/react-dom@16.12.0/index.js
So how you are using React in your build-less projects?

I'm a bit worried that we inventing new solutions to a known problem so I would prefer to copy solution that other libraries use for supporting build-less projects even if it requires bigger changes.

[lukejacksonn]

I am having to roll my own react at the moment (see https://github.com/lukejacksonn/es-react) which used to be created from the umd build which will have had anything to do with process stripped out of it already.

I'm not an expert in this kind of thing.. partially why I do bundle-less 😅 I just had a snoop around react/redux/vue for similar issue and found these relatable:

• Remove usage of process.env.NODE_ENV in ES module build reduxjs/redux#2907
• Check self.process before reading for browser compatibility reduxjs/redux#2910
• feat: add browser ESM build vuejs/vue#7110

Looking further I found this reduxjs/redux#3143 was merged (confirming my initial suspicion) which hints that we are going to need something like rollup-plugin-replace to transform these process.env statements at build time like:

replace({ 'process.env.NODE_ENV': JSON.stringify('production') })

Is the intention that the .mjs build we are generating be used in both esm compatible node and browser environment, or was it just intended for the browser?

[IvanGoncharov]

@lukejacksonn Thanks for the research 👍
Adding a transformation step is not a problem, the core issue here is that we can't apply it to mjs build since it's intended to be used by Node and should support NODE_ENV.
Adding separate browser build can be a solution as in the Redux case but this package is already 1.8Mb and I don't want to add a 5th copy of JS files and add another 600Kb.

But more importantly, it's not a long term solution since this check was added for a reason: we had a lot of issues from devs who were using duplicated instances of graphql package.

Also, we have a bunch of devAssert throughout our code so we also need to remove them from production builds but keep in dev builds.

So here is my proposal for long term plan:
Create a separate prod folder inside a package that doesn't contain any dev assertions. For example graphql/dev/language/parser (all dev check always) and graphql/prod/language/parser (no dev check at all).
So you can either have conditional imports in code or use something like import maps.
That way you can use both dev and prod with the build-less approach.
It's not something we can ship as part of upcoming 15.0.0 but definitely can include in future 16.0.0-alpha.1 (ETA 1-2 months).

What do you think?

P.S. Just to clarify my motives: I think graphql-tag should bundle graphql/language/parser and don't require an entire library just for the parser.

That said Deno also has the same problem of not having process: denoland/deno#2014
So I think it worth to figure out how to solve this problem for browser and all browser-like environments (atm only Deno but I think more comming in the future, e.g https://github.com/IBM/browser-functions).
But I don't want development environment to be a feature exclusive only to Node so we need to support both dev and prod environments on all platforms.

[lukejacksonn]

Hey Ivan, I have been on vacation the past week. Just catching up on GitHub notifications now..

But I don't want development environment to be a feature exclusive only to Node so we need to support both dev and prod environments on all platforms.

Yes, I presumed that that might be the blocker here. That considered, your proposal makes sense..

In the meantime (given that all the other esm compatibility issues have been resolved) one could make this work in the browser by creating a global variable process with a env attribute. It is of course not standard practice.. but it should stop the browser erroring. Something like:

window.process = {
  env: { 'NODE_ENV': 'production' }
};

When I get a moment I will give this a go and see what happens!

[IvanGoncharov]

@lukejacksonn Thanks for understanding 👍
and special thanks for your fight for a simple build environment ❤️

I maintain a few projects that have frontend code and I constantly struggling with updating Webpack or one of its numerous plugins.
So I totally support your cause and agree that adoption should start from the core packages like graphql just think we need a proper solution without losing functionality.

When I get a moment I will give this a go and see what happens!

It would be useful for everyone else searching for a temporary solution.

[lukejacksonn]

Thank you @IvanGoncharov for all your hard work.. people like you are truly an inspiration! Very happy to have you on board the cause 🚂 you have been exceptionally responsive here which I am very grateful for.

Will keep you posted on the workaround!

[n1ru4l]

The following workaround works fine on Chrome:

<script>
  window.process = globalThis.process = {
    env: {
      NODE_ENV: "development"
    }
  };
</script>

My use-case is a custom GraphiQL fetcher, which works quite nicely with tools like unpkg.com.

globalThis is available in all major js engines nowadays. Couldn't a pragmatic option, for now, be to make the process.env check fail-safe in browser environments (what this PR tries to solve) and additionally support some globalThis option (e.g. property check) for now. IMHO having something that runs without additional shims is still an improvement, even if it is not optimal (as there is no unified prod/dev env detection).

[yaacovCR]

Couldn't a pragmatic option, for now, be to make the process.env check fail-safe in browser environments (what this PR tries to solve) and additionally support some globalThis option (e.g. property check) for now. IMHO having something that runs without additional shims is still an improvement, even if it is not optimal (as there is no unified prod/dev env detection).

i think that makes sense @IvanGoncharov ?

[n1ru4l]

I created #3501