Add caching of process.env.NODE_ENV, for performance reasons

[jimrandomh]

apollo-client uses process.env.NODE_ENV to determine whether it's running in a production, development, or test environment. Unfortunately it turns out that polling process.env is slow; when I profiled (server side), our app spent ~100ms just on reading process.env.NODE_ENV (more time than on the features gated behind it!).

This PR adds basic caching, reading the environment variable once rather than rereading it frequently inside inner loops, reducing that component to a negligible amount of time spent. While it's theoretically possible for environment variables to change at runtime, this doesn't happen in practice (other than in the unit tests of the environment-variable reading).

[apollo-cla]

@jimrandomh: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[jimrandomh]

(I'll fix up merge conflicts and a unit test failure later today.)

[henryqdineen]

FWIW I believe this will negatively affect bundle size for us. We are relying on Webpack's built in optimizations and our minifier to do dead code elimination. As far as I know webpack and the minifier is not sophisticated enough to know the value of !isProduction() in that static context. Please let me know if I'm mistaken. Thanks.

[jimrandomh]

Interesting; we're using Apollo in Meteor, rather than Webpack (though trying to migrate away from Meteor eventually). That might explain how the (surprisingly large) performance impact went unnoticed, if Webpack is optimizing it away.

But note that this has performance impact both server-side and client-side, so if you're using optimization.nodeEnv for the client only, you're still taking a performance hit during SSR.

[jimrandomh]

Resolved merge conflicts, unit tests passing. Ping?

I was reminded of this when looking at our site profiler results today. It's still adding 100ms to our SSR times on lesswrong.com, and I would really like to see this (or something with the same effect) get merged. I understand that Webpack has some automagic optimization that works well here for the client, and this adds a tiny bit to the bundle size, but I believe it's worth it. The performance we've been getting out of apollo-client has been pretty bad, and this issue is a significant part of that.

[benjamn]

@jimrandomh Sorry for the wait on this!

Per #7523 (comment), the failure of the Filesize test is indeed legitimate, because the process.env.NODE_ENV syntax is what most minifiers in the JS/React ecosystem are looking for when they do dead code elimination, so changing that syntax dramatically increases the @apollo/client bundle size.

I wish the available minifiers were smarter about function inlining and constant propagation, so that something like isProduction() could be turned into a constant—or that React had not cemented this process.env.NODE_ENV convention—but we're unfortunately stuck with this particular set of tools and conventions for now.

Working with the convention, though, perhaps there's a way we could override/shadow the process variable in Apollo Client modules that examine process.env.NODE_ENV, so the access does not always have to reread the environment?

[jimrandomh]

Looks like #7627 solves the issue. Thanks!