urql-graphql / urql-devtools-exchange

The exchange for usage with Urql Devtools
https://www.npmjs.com/package/@urql/devtools
MIT License
54 stars 6 forks source link

Uncaught ReferenceError: process is not defined #37

Closed frederikhors closed 4 years ago

frederikhors commented 4 years ago

I'm using @rollup/plugin-replace:

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

but I get this error using @urql/svelte:

urql-devtools-exchange.es.js:46 Uncaught ReferenceError: process is not defined
    at devtoolsExchange (urql-devtools-exchange.es.js:46)
    at urql-core.mjs:329
    at Array.reduceRight (<anonymous>)
    at urql-core.mjs:328
    at new Z$1 (urql-core.mjs:418)
    at initClient (urql-svelte.mjs:63)
    at Object.OnInit (urql.js:162)
    at onInit (actions.js:14)
    at instance$q (App.svelte:14)
    at init (index.mjs:1409)
ryan-roemer commented 4 years ago

Looking at the dist code:

  if ("undefined" == typeof window || "production" === (null === (_a = null === process || void 0 === process ? void 0 : process.env) || void 0 === _a ? void 0 : _a.NODE_ENV)) {
    return function _ref(ops$) {
      return pipe(ops$, forward);
    };
  }

I'm guessing:

  1. @rollup/plugin-replace isn't sophisticated enough to parse that tortured long statement.
  2. There's no actual guard around process in the transpiled code. I think at a minimum we need a typeof process !== "undefined"
ryan-roemer commented 4 years ago

The underlying code is: https://github.com/FormidableLabs/urql-devtools-exchange/blob/master/src/exchange.ts#L24

if (
  typeof window === 'undefined' ||
  process?.env?.NODE_ENV === 'production'
)

And that makes sense for all that transpiled gibberish. Something simple that doesn't throw and avoids the chonkiness of ? operator could look like:

if (
  typeof window === 'undefined' ||
  (typeof process !== 'undefined' && ((process || {}).env || {}).NODE_ENV === 'production')
)
JoviDeCroock commented 4 years ago

I've been reasoning more often about this problem lately, the hard part about supporting the optimization of those __DEV__ checks is that if we make them optional to support bundler-less environments that this optimization starts failing.

In practice we transform from

if (process.env.NODE_ENV === 'production) {
  return;
}

// Code here can be removed when the above is true

So in our production step this tranforms to

if ('production' === 'production) {
  return;
}

Making Terser remove it, this however isn't the case anymore if we'd add process && there because the replace step only takes the full string to be replaced by the stringified value.

An option would be to expose a browser-dev and browser-prod build which used to be done for UMD this however conflicts with the package.json standards. It's a problem we'll have to solve in urql as well when it pops up. For now we still have process.env.NODE_ENV without the process check.

andyrichardson commented 4 years ago

So it sounds like there's more to this than expected - let's add a check for the existence of process first and then go from there?

If process doesn't exist, I'll assume we're not in a dev build. Maybe this is a good justification for having some kind of force argument for those not using a build process where process is present (such as esmodule builds).

andyrichardson commented 4 years ago

I've gone ahead and removed any logic for esmodule / buildless support which should resolve this issue - given urql doesn't support buildless environments anyhow nobody should be affected.

Keep you eyes peeled for a release some time within the next week.

andyrichardson commented 4 years ago

Quick update on this, I think we're going to be able to add back support for esmodules thanks to @JoviDeCroock's work on urql core - https://github.com/FormidableLabs/urql/pull/827

We'll be looking at something like this which should work nicely with webpack / terser thanks to the use of the && operator rather than || (which was preventing dead code removal due to the inability to do string replacement / evaluate the second part of the conditional).

if (
  typeof window === 'undefined' ||
  (typeof process !== undefined && process.env.NODE_ENV === 'production')
)

esmodule result: false (no "production" capability in esmodule for now) build process result: true