Prevent ReferenceError when running graphql in the browser

[bennypowers]

Not every user wants to or should use webpack to bundle graphql and related libs.
Forcing web developers to cludge a global reference to an imaginary process in a render-blocking script is not an optimal solution, when libraries can just check it themselves.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[Neitsch]

Will glbl.process be undefined on the client, so that it always evaluates to false? I understand where this is coming from. Let's make sure that it's a safe change :)

[bennypowers]

@Neitsch would you prefer to also check for glbl.process.env and global.process.env.NODE_ENV ?

Also, what constitutes a safe change?

literally anything is better than immediately throwing when trying to access env on undefined

[Neitsch]

Oops, sorry, I was thinking about something wrong. Yeah, this looks like a fine idea.

Could you elaborate in which case you would not have process available? A node process will always have process defined. Do you want to run this code on the client / browser?

[bennypowers]

Yes. I want to run this in the browser, and I don't want to solve this problem with added tooling.

[bennypowers]

👋 Any reasons not to merge this?

Here's an example use case:

[IvanGoncharov]

@bennypowers Sorry for delay and thanks for the ping.

Here's an example use case:

Example code is missing in your comment for some reason.

Yes. I want to run this in the browser, and I don't want to solve this problem with added tooling.

Just to better understand your use case, how you handle require inside graphql JS files without webpack or other bundlers?

[bennypowers]

Example code is missing in your comment for some reason.

whoopsie! sleep is good, I promise.

sorry for the long story here but it's genuinely convoluted.

https://lit-apollo-subscriptions.herokuapp.com uses @apollo-elements/lit-apollo to define and render components.

lit-apollo depends on apollo-client, which has a peer dependency for graphql

so when instanceOf.mjs loads up in the browser, it hits that ternary and crashes the app.

I handle require with rollup-plugin-commonjs, but prefer to use es modules for treeshaking and bundle size reasons, and because they are native to the browser.

So, I could solve this by importing builtins from rollup-plugin-globals or something, but I think that's a poor choice for many reasons:

1. it pointlessly inflates the bundle size
2. it doesn't add value to the app in any way asides from preventing this error
3. it strikes me as odd, and way overkill to apply that level of tooling when a simple check would suffice
4. even if some preferred tool would come along and solve the above, why limit users to a specific toolchain? surely the goal should be to reduce the amount of required tooling

So that leaves me with the solution here:

https://gitlab.com/bennyp/demo-lit-apollo-subscriptions/blob/master/src/index.html#L32

Which makes me a little sad. Why assume node-everywhere when, for the price of 44 characters of straightforward, good-old-fashioned runtime javascript, you can cover node and web cases.

I hope that makes sense.

[IvanGoncharov]

@bennypowers Thanks for detailed explanation now its totally makes sense 👍
One problem though is that we spend a few iterations to make this code work properly with different bundlers and other tools.
AFAIK many such tools rely on static code analysis to figure out code patterns so it's hard to predict if we break something or not.

The ideal solution would be to find some popular library that faces the same problems and copy-paste code from here. This way we can ensure that this code is battle tested with the majority of setups/stacks without putting graphql-js community through a new cycle of releases that break stuff that worked previously.

For example, I did a quick check for react-dom and it looks like they have a separate build for NPM that have an exact same check as we have ATM:
https://github.com/facebook/react/blob/baff5cc2f69d30589a5dc65b089e47765437294b/packages/react-dom/npm/index.js#L31
I never used React without webpack so I not sure how you would use React in such case.

So if you have the time it would be great if you find out how other libraries solve this issue and save us from reinventing the wheel.

[bennypowers]

in the node context this will have the exact same behavior as before

It will hit the try block, then evaluate the expression

glbl = global;

when it hits the if statement, glbl.process && glbl.process.env.NODE_ENV will evaluate the exact same as process.env

proof

The difference here is that it won't cause a ReferenceError on the web.

The only danger with this approach is if someone defines window.global.process or something.

proof of error (open console)
proof of fix

[bennypowers]

But here are some more comprehensive thoughts on the matter
https://github.com/tc39/proposal-global#rationale

[bennypowers]

I pushed a commit which uses the expanded getGlobal from es6-shim
paulmillr/es6-shim@2367e09#commitcomment-27989605

In the future, I suppose globalThis would do the trick.

[Neitsch]

As a hypothetical: What is the advantage of your solution over just doing `process && process.env && process.env.NODE_ENV == "production"? To use production mode you can set the global variable appropriately.

On a side note, I would strongly advise declaring process.env.NODE_ENV = 'production', since this can reduce bundle size.

[bennypowers]

I originally did that, but a more comprehensive solution was sought

[IvanGoncharov]

Superseded by #2409