Skip to content

Commit

Permalink
Experiment: use __DEV__ instead of process.env.NODE_ENV.
Browse files Browse the repository at this point in the history
As explained in #8266, our use of process.env.NODE_ENV requires those
expressions to be either replaced with a string literal by a minifier
(common convention in the React ecosystem) or polyfilled globally.

We stopped polyfilling process.env globally in the ts-invariant package
in apollographql/invariant-packages#94, but
@apollo/client is still relying on process.env internally, as is the
graphql package. If we want to rid ourselves fully of the drawbacks of
process.env.NODE_ENV, we probably ought to stop using it ourselves.

Though most developers in the React ecosystem will be using a bundler or
minifier that replaces process.env.NODE_ENV at build time, you may not
have the luxury of custom minification when loading @apollo/client from
a CDN, which leaves only the option of a global process polyfill, which
is problematic because that's how some applications detect if the
current code is running Node.js (more details/links in #8266).

Instead, I believe we can (and must?) stop using process.env.NODE_ENV,
and one of many better alternatives appears to be the __DEV__ variable
used by React Native, which is much easier to polyfill, since it's a
single variable rather than a nested object.

Switching to __DEV__ will initially cause a large bundle size regression
(+3.5Kb *after* minification and gzip), but that's not technically a
breaking change (and thus acceptable for AC3.4), and it should be easy
to reconfigure minifiers to replace __DEV__ with true or false (or even
just undefined), with sufficient guidance from the release notes.

That still leaves the process.env.NODE_ENV check in instanceOf.mjs in
the graphql package. Discussion in graphql/graphql-js#2894
suggests the plan is to stop using NODE_ENV altogether, which would be
ideal, but that won't happen until graphql@16 at the earliest.

To work around the problem in the meantime, I devised a strategy where
we polyfill process.env.NODE_ENV only briefly, while we import code that
depends on graphql/jsutils/instanceOf.mjs, and then synchronously remove
the global polyfill, so it does not permanently pollute the global
namespace.

This strategy assumes @apollo/client is the first to import the graphql
package. If you have other code that imports instanceOf.mjs earlier, and
you don't have a process.env.NODE_ENV strategy already, it's your
responsibility to make that import work, however you see fit. Apollo
Client is only responsible for making sure its own imports of the
graphql package have a chance of succeeding, a responsibility I believe
my strategy fulfills cleverly if not elegantly.

Of course, this charade does not happen if process.env.NODE_ENV is
already defined or has been minified away, but only if accessing it
would throw, since that's what we're trying to avoid.

Although we could do some more work to reduce the bundle size impact of
blindly switching to __DEV__, I believe this PR already solves the last
remaining hurdles documented in #8266, potentially allowing
@apollo/client/core@beta to be loaded from an ESM-aware CDN like esm.run
or jspm.io. The default __DEV__ value will be true in those
environments, but could be initialized differently by a script/module
that runs earlier in the HTML of the page.
  • Loading branch information
benjamn committed Jun 3, 2021
1 parent 14ffe47 commit eb1532c
Show file tree
Hide file tree
Showing 23 changed files with 124 additions and 141 deletions.
45 changes: 17 additions & 28 deletions config/processInvariants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,50 +71,50 @@ function transform(code: string, file: string) {
const node = path.node;

if (isCallWithLength(node, "invariant", 1)) {
if (isNodeEnvConditional(path.parent.node)) {
if (isDEVConditional(path.parent.node)) {
return;
}

const newArgs = node.arguments.slice(0, 1);
newArgs.push(getErrorCode(file, node));

return b.conditionalExpression(
makeNodeEnvTest(),
makeDEVExpr(),
node,
b.callExpression.from({
...node,
arguments: newArgs,
}),
node,
);
}

if (node.callee.type === "MemberExpression" &&
isIdWithName(node.callee.object, "invariant") &&
isIdWithName(node.callee.property, "log", "warn", "error")) {
if (isNodeEnvLogicalOr(path.parent.node)) {
if (isDEVLogicalAnd(path.parent.node)) {
return;
}
return b.logicalExpression("||", makeNodeEnvTest(), node);
return b.logicalExpression("&&", makeDEVExpr(), node);
}
},

visitNewExpression(path) {
this.traverse(path);
const node = path.node;
if (isCallWithLength(node, "InvariantError", 0)) {
if (isNodeEnvConditional(path.parent.node)) {
if (isDEVConditional(path.parent.node)) {
return;
}

const newArgs = [getErrorCode(file, node)];

return b.conditionalExpression(
makeNodeEnvTest(),
makeDEVExpr(),
node,
b.newExpression.from({
...node,
arguments: newArgs,
}),
node,
);
}
}
Expand All @@ -137,32 +137,21 @@ function isCallWithLength(
node.arguments.length > length;
}

function isNodeEnvConditional(node: Node) {
function isDEVConditional(node: Node) {
return n.ConditionalExpression.check(node) &&
isNodeEnvExpr(node.test);
isDEVExpr(node.test);
}

function isNodeEnvLogicalOr(node: Node) {
function isDEVLogicalAnd(node: Node) {
return n.LogicalExpression.check(node) &&
node.operator === "||" &&
isNodeEnvExpr(node.left);
node.operator === "&&" &&
isDEVExpr(node.left);
}

function makeNodeEnvTest() {
return b.binaryExpression(
"===",
b.memberExpression(
b.memberExpression(
b.identifier("process"),
b.identifier("env")
),
b.identifier("NODE_ENV"),
),
b.stringLiteral("production"),
);
function makeDEVExpr() {
return b.identifier("__DEV__");
}

const referenceNodeEnvExpr = makeNodeEnvTest();
function isNodeEnvExpr(node: Node) {
return recast.types.astNodesAreEquivalent(node, referenceNodeEnvExpr);
function isDEVExpr(node: Node) {
return isIdWithName(node, "__DEV__");
}
23 changes: 1 addition & 22 deletions config/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ function prepareCJSMinified(input) {
compress: {
toplevel: true,
global_defs: {
'@process.env.NODE_ENV': JSON.stringify('production'),
'@__DEV__': 'false',
},
},
}),
Expand All @@ -97,27 +97,6 @@ function prepareBundle({
exports: 'named',
interop: 'esModule',
externalLiveBindings: false,
// In Node.js, where these CommonJS bundles are most commonly used,
// the expression process.env.NODE_ENV can be very expensive to
// evaluate, because process.env is a wrapper for the actual OS
// environment, and lookups are not cached. We need to preserve the
// syntax of process.env.NODE_ENV expressions for dead code
// elimination to work properly, but we can apply our own caching by
// shadowing the global process variable with a stripped-down object
// that saves a snapshot of process.env.NODE_ENV when the bundle is
// first evaluated. If we ever need other process properties, we can
// add more stubs here.
intro: '!(function (process) {',
outro: [
'}).call(this, {',
' env: {',
' NODE_ENV: typeof process === "object"',
' && process.env',
' && process.env.NODE_ENV',
' || "development"',
' }',
'});',
].join('\n'),
},
plugins: [
extensions ? nodeResolve({ extensions }) : nodeResolve(),
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/__snapshots__/exports.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Array [
"NetworkStatus",
"Observable",
"ObservableQuery",
"__DEV__",
"checkFetcher",
"concat",
"createHttpLink",
Expand Down Expand Up @@ -67,6 +68,7 @@ Array [
"InMemoryCache",
"MissingFieldError",
"Policies",
"__DEV__",
"cacheSlot",
"canonicalStringify",
"defaultDataIdFromObject",
Expand All @@ -90,6 +92,7 @@ Array [
"NetworkStatus",
"Observable",
"ObservableQuery",
"__DEV__",
"checkFetcher",
"concat",
"createHttpLink",
Expand Down Expand Up @@ -224,6 +227,7 @@ Array [
"ApolloConsumer",
"ApolloProvider",
"DocumentType",
"__DEV__",
"getApolloContext",
"operationName",
"parser",
Expand Down Expand Up @@ -320,6 +324,7 @@ Array [
"Concast",
"DeepMerger",
"Observable",
"__DEV__",
"addTypenameToDocument",
"argumentsObjectFromField",
"asyncMap",
Expand Down
2 changes: 2 additions & 0 deletions src/cache/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
export { __DEV__ } from "../utilities";

export { Transaction, ApolloCache } from './core/cache';
export { Cache } from './core/types/Cache';
export { DataProxy } from './core/types/DataProxy';
Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/__tests__/roundtrip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ function storeRoundtrip(query: DocumentNode, result: any, variables = {}) {
const immutableResult = readQueryFromStore(reader, readOptions);
expect(immutableResult).toEqual(reconstructedResult);
expect(readQueryFromStore(reader, readOptions)).toBe(immutableResult);
if (process.env.NODE_ENV !== 'production') {
if (__DEV__) {
try {
// Note: this illegal assignment will only throw in strict mode, but that's
// safe to assume because this test file is a module.
Expand Down
4 changes: 2 additions & 2 deletions src/cache/inmemory/object-canon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export class ObjectCanon {
// Since canonical arrays may be shared widely between
// unrelated consumers, it's important to regard them as
// immutable, even if they are not frozen in production.
if (process.env.NODE_ENV !== "production") {
if (__DEV__) {
Object.freeze(array);
}
}
Expand Down Expand Up @@ -154,7 +154,7 @@ export class ObjectCanon {
// Since canonical objects may be shared widely between
// unrelated consumers, it's important to regard them as
// immutable, even if they are not frozen in production.
if (process.env.NODE_ENV !== "production") {
if (__DEV__) {
Object.freeze(obj);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ export class StoreReader {
}), i);
}

if (process.env.NODE_ENV !== 'production') {
if (__DEV__) {
assertSelectionSetForIdValue(context.store, field, item);
}

Expand Down
4 changes: 2 additions & 2 deletions src/cache/inmemory/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ export class StoreWriter {
incomingFields = this.applyMerges(mergeTree, entityRef, incomingFields, context);
}

if (process.env.NODE_ENV !== "production" && !context.overwrite) {
if (__DEV__ && !context.overwrite) {
const hasSelectionSet = (storeFieldName: string) =>
fieldsWithSelectionSets.has(fieldNameFromStoreName(storeFieldName));
const fieldsWithSelectionSets = new Set<string>();
Expand Down Expand Up @@ -319,7 +319,7 @@ export class StoreWriter {
// In development, we need to clone scalar values so that they can be
// safely frozen with maybeDeepFreeze in readFromStore.ts. In production,
// it's cheaper to store the scalar values directly in the cache.
return process.env.NODE_ENV === 'production' ? value : cloneDeep(value);
return __DEV__ ? cloneDeep(value) : value;
}

if (Array.isArray(value)) {
Expand Down
4 changes: 2 additions & 2 deletions src/core/ApolloClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export class ApolloClient<TCacheShape> implements DataProxy {
// devtools, but disable them by default in production.
typeof window === 'object' &&
!(window as any).__APOLLO_CLIENT__ &&
process.env.NODE_ENV !== 'production',
__DEV__,
queryDeduplication = true,
defaultOptions,
assumeImmutableResults = false,
Expand Down Expand Up @@ -204,7 +204,7 @@ export class ApolloClient<TCacheShape> implements DataProxy {
/**
* Suggest installing the devtools for developers who don't have them
*/
if (!hasSuggestedDevtools && process.env.NODE_ENV !== 'production') {
if (!hasSuggestedDevtools && __DEV__) {
hasSuggestedDevtools = true;
if (
typeof window !== 'undefined' &&
Expand Down
2 changes: 1 addition & 1 deletion src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ export class ObservableQuery<
const { updateQuery } = fetchMoreOptions;

if (updateQuery) {
if (process.env.NODE_ENV !== "production" &&
if (__DEV__ &&
!warnedAboutUpdateQuery) {
invariant.warn(
`The updateQuery callback for fetchMore is deprecated, and will be removed
Expand Down
4 changes: 2 additions & 2 deletions src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1307,7 +1307,7 @@ export class QueryManager<TStore> {
) => {
const data = diff.result;

if (process.env.NODE_ENV !== 'production' &&
if (__DEV__ &&
isNonEmptyArray(diff.missing) &&
!equal(data, {}) &&
!returnPartialData) {
Expand Down Expand Up @@ -1464,7 +1464,7 @@ function getQueryIdsForQueryDescriptor<TStore>(
// pre-allocate a new query ID here.
queryIds.push(qm.generateQueryId());
}
if (process.env.NODE_ENV !== "production" && !queryIds.length) {
if (__DEV__ && !queryIds.length) {
invariant.warn(`Unknown query name ${
JSON.stringify(desc)
} passed to refetchQueries method in options.include array`);
Expand Down
2 changes: 2 additions & 0 deletions src/core/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/* Core */

export { __DEV__ } from "../utilities";

export {
ApolloClient,
ApolloClientOptions,
Expand Down
2 changes: 2 additions & 0 deletions src/react/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
export { __DEV__ } from "../utilities";

export {
ApolloProvider,
ApolloConsumer,
Expand Down
58 changes: 0 additions & 58 deletions src/utilities/common/__tests__/environment.ts

This file was deleted.

20 changes: 0 additions & 20 deletions src/utilities/common/environment.ts

This file was deleted.

15 changes: 15 additions & 0 deletions src/utilities/common/global.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { maybe } from "./maybe";

declare global {
const __DEV__: boolean | undefined;
}

export default (
maybe(() => globalThis) ||
maybe(() => window) ||
maybe(() => self) ||
maybe(() => global) ||
maybe(() => Function("return this")())
) as typeof globalThis & {
__DEV__: typeof __DEV__;
};
3 changes: 3 additions & 0 deletions src/utilities/common/maybe.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function maybe<T>(thunk: () => T): T | undefined {
try { return thunk() } catch {}
}
Loading

0 comments on commit eb1532c

Please sign in to comment.