(babel) - Fix ESM support by adding typeof process check

[JoviDeCroock]

Add a transform that gets rid of all the process.env.NODE_ENV !== 'production' checks and transforms them into typeof process !== 'undefined' && process.env.NODE_ENV !== 'production'.

This is an issue we initially faced in devtools where we tried to introduce conditionals process?.env?.NODE_ENV which will result in the same esm-incompatability where the global isn't correctly referenced.

I've tested and this check is also correctly DCE'd by terser and closure.

Try it out

const x = () => {
  if (typeof process !== 'undefined' && 'production' !== 'production') {
    console.log('y')    
  }
}

x();

See it in action

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 7fcfc02

We got this.

This PR includes changesets to release 8 packages

Name	Type
@urql/exchange-graphcache	Minor
@urql/exchange-multipart-fetch	Minor
@urql/exchange-persisted-fetch	Minor
@urql/exchange-populate	Minor
@urql/exchange-suspense	Minor
@urql/core	Minor
@urql/preact	Minor
urql	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[andyrichardson]

This is awesome @JoviDeCroock! Really glad to see we're adding in esmodule support.

One thing worth emphasizing is that we don't currently have a way of switching between prod / dev builds when using esmodules (not necessarily for optimization purposes but useful for removing developer warnings and prompts).

As it stands right now, we default to dev for esmodule which IMHO makes sense. If there's some kind of esmodule standard for flagging the environment the user is working in, we could definitely support that in the future 👍

Super basic example of how someone people might be doing this when deploying

// index.js
window._ES_PROD_ = false;

// On prod deployment script
sed -i 's/window._ES_PROD_ = false/window._ES_PROD_ = true/' index.js
// ...

[lukejacksonn]

Great work @JoviDeCroock 🥳 and yes.. Andy raises a good point. There currently isn't a there isn't any kind of esmodule standard for flagging the environment the user is working in unfortunately.

This issue has plagued a few other repos and prevented them becoming fully browser esm compatible like what is being proposed here.

Most of the research I have done into this issue is documented in this PR on graphql-js itself graphql/graphql-js#2409 which was the last piece in this puzzle graphql/graphql-js#2277.

Perhaps we should consider speccing out what a standard for this could look like!

[kitten]

Seems like we’re only missing a changeset here 👍

[kitten]

Small typo in the filename here: compatability => compatibility
Otherwise only the changeset is missing, but I suppose we can just patch bump everything

[JoviDeCroock]

I've been trying to test some stuff and it seems to work but we haven't got sandboxes for everything 😅 I think this code can't actually negatively impact any places judging from the astexplorer though. If anyone comes up with an edge case please throw it at me.

I'll add the changesets later

[amyboyd]

This looks like a neat addition!

Does it also handle a === as well as !==? For example if (process.env.NODE_ENV === 'production') { return; } doStuff();

[JoviDeCroock]

@amyboyd yes it does but for urql doesn't make much of a difference because commonly in bundlers the __dev__ optimization is process.env.NODE_ENV !== 'production' this because when the replacePlugin of a bundler kicks in this becomes 'production' !== 'production' which when your minification comes into play (Terser, ...) is removed because this code is unreachable all of a sudden.

This optimization is commonly used in urql to give developer warnings to the user, these developer warnings however are useless in production and bloat the bundle size of the app in question (since most of the times these are long strings).

• Removal of the debug-events code can be found here
• Transformation of the invariants and warnings can be found here

In libraries there probably won't be a case for process.env.NODE_ENV === 'production' since it serves no purpose of being cut out in development.

[amyboyd]

OK, I think I follow. With lines like 144 here, will the if need inverted? Same for line 197, 238?

[JoviDeCroock]

It shouldn't be a hard requirement, I do it because specifying the env isn't as standard in for instance svelte (we've had a couple of issues about that) which means that those could trigger in a published application.

Because we'd bail out when "production" === "production" for the case where there's no env supplied this will never bail and produce those errors in prod.

[kitten]

The only case here that's uncovered is that it's not transpiling process.env.NODE_ENV !== 'production' to typeof process === 'undefined || process.env.NODE_ENV !== 'production'

[kitten]

Temporarily closing this until we have a solid solution for ESM dev-checks that compile away