From eb1532ccc7ca8a4c4d59664bb7bd8cdd5bb43015 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 27 May 2021 17:13:04 -0400 Subject: [PATCH] Experiment: use __DEV__ instead of process.env.NODE_ENV. 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 https://github.com/apollographql/invariant-packages/pull/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 https://github.com/graphql/graphql-js/pull/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. --- config/processInvariants.ts | 45 ++++++-------- config/rollup.config.js | 23 +------- src/__tests__/__snapshots__/exports.ts.snap | 5 ++ src/cache/index.ts | 2 + src/cache/inmemory/__tests__/roundtrip.ts | 2 +- src/cache/inmemory/object-canon.ts | 4 +- src/cache/inmemory/readFromStore.ts | 2 +- src/cache/inmemory/writeToStore.ts | 4 +- src/core/ApolloClient.ts | 4 +- src/core/ObservableQuery.ts | 2 +- src/core/QueryManager.ts | 4 +- src/core/index.ts | 2 + src/react/index.ts | 2 + src/utilities/common/__tests__/environment.ts | 58 ------------------- src/utilities/common/environment.ts | 20 ------- src/utilities/common/global.ts | 15 +++++ src/utilities/common/maybe.ts | 3 + src/utilities/common/maybeDeepFreeze.ts | 4 +- src/utilities/fixes/__DEV__.ts | 18 ++++++ src/utilities/fixes/graphql.ts | 7 +++ src/utilities/fixes/index.ts | 3 + src/utilities/fixes/process.ts | 34 +++++++++++ src/utilities/index.ts | 2 + 23 files changed, 124 insertions(+), 141 deletions(-) delete mode 100644 src/utilities/common/__tests__/environment.ts delete mode 100644 src/utilities/common/environment.ts create mode 100644 src/utilities/common/global.ts create mode 100644 src/utilities/common/maybe.ts create mode 100644 src/utilities/fixes/__DEV__.ts create mode 100644 src/utilities/fixes/graphql.ts create mode 100644 src/utilities/fixes/index.ts create mode 100644 src/utilities/fixes/process.ts diff --git a/config/processInvariants.ts b/config/processInvariants.ts index c719d5b71bf..0a26a01e9ee 100644 --- a/config/processInvariants.ts +++ b/config/processInvariants.ts @@ -71,7 +71,7 @@ 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; } @@ -79,22 +79,22 @@ function transform(code: string, file: string) { 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); } }, @@ -102,19 +102,19 @@ function transform(code: string, file: string) { 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, ); } } @@ -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__"); } diff --git a/config/rollup.config.js b/config/rollup.config.js index abe73fabcd2..511c832acfc 100644 --- a/config/rollup.config.js +++ b/config/rollup.config.js @@ -71,7 +71,7 @@ function prepareCJSMinified(input) { compress: { toplevel: true, global_defs: { - '@process.env.NODE_ENV': JSON.stringify('production'), + '@__DEV__': 'false', }, }, }), @@ -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(), diff --git a/src/__tests__/__snapshots__/exports.ts.snap b/src/__tests__/__snapshots__/exports.ts.snap index f5acde2fc1e..2f5236a1156 100644 --- a/src/__tests__/__snapshots__/exports.ts.snap +++ b/src/__tests__/__snapshots__/exports.ts.snap @@ -16,6 +16,7 @@ Array [ "NetworkStatus", "Observable", "ObservableQuery", + "__DEV__", "checkFetcher", "concat", "createHttpLink", @@ -67,6 +68,7 @@ Array [ "InMemoryCache", "MissingFieldError", "Policies", + "__DEV__", "cacheSlot", "canonicalStringify", "defaultDataIdFromObject", @@ -90,6 +92,7 @@ Array [ "NetworkStatus", "Observable", "ObservableQuery", + "__DEV__", "checkFetcher", "concat", "createHttpLink", @@ -224,6 +227,7 @@ Array [ "ApolloConsumer", "ApolloProvider", "DocumentType", + "__DEV__", "getApolloContext", "operationName", "parser", @@ -320,6 +324,7 @@ Array [ "Concast", "DeepMerger", "Observable", + "__DEV__", "addTypenameToDocument", "argumentsObjectFromField", "asyncMap", diff --git a/src/cache/index.ts b/src/cache/index.ts index 5e3f221a44f..220bfdf378a 100644 --- a/src/cache/index.ts +++ b/src/cache/index.ts @@ -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'; diff --git a/src/cache/inmemory/__tests__/roundtrip.ts b/src/cache/inmemory/__tests__/roundtrip.ts index 8facc6cbbf5..8604a09a883 100644 --- a/src/cache/inmemory/__tests__/roundtrip.ts +++ b/src/cache/inmemory/__tests__/roundtrip.ts @@ -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. diff --git a/src/cache/inmemory/object-canon.ts b/src/cache/inmemory/object-canon.ts index e436d74d7dd..22f39b4243d 100644 --- a/src/cache/inmemory/object-canon.ts +++ b/src/cache/inmemory/object-canon.ts @@ -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); } } @@ -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); } } diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index dbce333b190..7fcfa59b343 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -477,7 +477,7 @@ export class StoreReader { }), i); } - if (process.env.NODE_ENV !== 'production') { + if (__DEV__) { assertSelectionSetForIdValue(context.store, field, item); } diff --git a/src/cache/inmemory/writeToStore.ts b/src/cache/inmemory/writeToStore.ts index d50f54ecdec..4317aae944e 100644 --- a/src/cache/inmemory/writeToStore.ts +++ b/src/cache/inmemory/writeToStore.ts @@ -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(); @@ -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)) { diff --git a/src/core/ApolloClient.ts b/src/core/ApolloClient.ts index 53485777b07..117dabdb1e5 100644 --- a/src/core/ApolloClient.ts +++ b/src/core/ApolloClient.ts @@ -150,7 +150,7 @@ export class ApolloClient 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, @@ -204,7 +204,7 @@ export class ApolloClient 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' && diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 998cbb9577a..cbc9b964106 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -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 diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index cb08575deb5..936ff0ad3be 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1307,7 +1307,7 @@ export class QueryManager { ) => { const data = diff.result; - if (process.env.NODE_ENV !== 'production' && + if (__DEV__ && isNonEmptyArray(diff.missing) && !equal(data, {}) && !returnPartialData) { @@ -1464,7 +1464,7 @@ function getQueryIdsForQueryDescriptor( // 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`); diff --git a/src/core/index.ts b/src/core/index.ts index c5c14217944..8e01d030b4c 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -1,5 +1,7 @@ /* Core */ +export { __DEV__ } from "../utilities"; + export { ApolloClient, ApolloClientOptions, diff --git a/src/react/index.ts b/src/react/index.ts index a423c3c75a1..3d9c5583b9e 100644 --- a/src/react/index.ts +++ b/src/react/index.ts @@ -1,3 +1,5 @@ +export { __DEV__ } from "../utilities"; + export { ApolloProvider, ApolloConsumer, diff --git a/src/utilities/common/__tests__/environment.ts b/src/utilities/common/__tests__/environment.ts deleted file mode 100644 index f9fe25362db..00000000000 --- a/src/utilities/common/__tests__/environment.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { isEnv, isDevelopment, isTest } from '../environment'; - -describe('environment', () => { - let keepEnv: string | undefined; - - beforeEach(() => { - // save the NODE_ENV - keepEnv = process.env.NODE_ENV; - }); - - afterEach(() => { - // restore the NODE_ENV - process.env.NODE_ENV = keepEnv; - }); - - describe('isEnv', () => { - it(`should match when there's a value`, () => { - ['production', 'development', 'test'].forEach(env => { - process.env.NODE_ENV = env; - expect(isEnv(env)).toBe(true); - }); - }); - - it(`should treat no proces.env.NODE_ENV as it'd be in development`, () => { - delete process.env.NODE_ENV; - expect(isEnv('development')).toBe(true); - }); - }); - - describe('isTest', () => { - it('should return true if in test', () => { - process.env.NODE_ENV = 'test'; - expect(isTest()).toBe(true); - }); - - it('should return true if not in test', () => { - process.env.NODE_ENV = 'development'; - expect(!isTest()).toBe(true); - }); - }); - - describe('isDevelopment', () => { - it('should return true if in development', () => { - process.env.NODE_ENV = 'development'; - expect(isDevelopment()).toBe(true); - }); - - it('should return true if not in development and environment is defined', () => { - process.env.NODE_ENV = 'test'; - expect(!isDevelopment()).toBe(true); - }); - - it('should make development as the default environment', () => { - delete process.env.NODE_ENV; - expect(isDevelopment()).toBe(true); - }); - }); -}); diff --git a/src/utilities/common/environment.ts b/src/utilities/common/environment.ts deleted file mode 100644 index 21d61f5f122..00000000000 --- a/src/utilities/common/environment.ts +++ /dev/null @@ -1,20 +0,0 @@ -export function getEnv(): string | undefined { - if (typeof process !== 'undefined' && process.env.NODE_ENV) { - return process.env.NODE_ENV; - } - - // default environment - return 'development'; -} - -export function isEnv(env: string): boolean { - return getEnv() === env; -} - -export function isDevelopment(): boolean { - return isEnv('development') === true; -} - -export function isTest(): boolean { - return isEnv('test') === true; -} diff --git a/src/utilities/common/global.ts b/src/utilities/common/global.ts new file mode 100644 index 00000000000..6464e27a3ca --- /dev/null +++ b/src/utilities/common/global.ts @@ -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__; +}; diff --git a/src/utilities/common/maybe.ts b/src/utilities/common/maybe.ts new file mode 100644 index 00000000000..dd9d4c90d80 --- /dev/null +++ b/src/utilities/common/maybe.ts @@ -0,0 +1,3 @@ +export function maybe(thunk: () => T): T | undefined { + try { return thunk() } catch {} +} diff --git a/src/utilities/common/maybeDeepFreeze.ts b/src/utilities/common/maybeDeepFreeze.ts index f8e9d483d1b..7cac2539b87 100644 --- a/src/utilities/common/maybeDeepFreeze.ts +++ b/src/utilities/common/maybeDeepFreeze.ts @@ -1,4 +1,4 @@ -import { isDevelopment, isTest } from './environment'; +import '../fixes'; // For __DEV__ import { isNonNullObject } from './objects'; function deepFreeze(value: any) { @@ -15,7 +15,7 @@ function deepFreeze(value: any) { } export function maybeDeepFreeze(obj: T): T { - if (process.env.NODE_ENV !== "production" && (isDevelopment() || isTest())) { + if (__DEV__) { deepFreeze(obj); } return obj; diff --git a/src/utilities/fixes/__DEV__.ts b/src/utilities/fixes/__DEV__.ts new file mode 100644 index 00000000000..560d692e64e --- /dev/null +++ b/src/utilities/fixes/__DEV__.ts @@ -0,0 +1,18 @@ +import global from "../common/global"; +import { maybe } from "../common/maybe"; + +function getDEV() { + try { + return Boolean(__DEV__); + } catch { + Object.defineProperty(global, "__DEV__", { + value: maybe(() => process.env.NODE_ENV) !== "production", + enumerable: false, + configurable: true, + writable: true, + }); + return global.__DEV__; + } +} + +export default getDEV(); diff --git a/src/utilities/fixes/graphql.ts b/src/utilities/fixes/graphql.ts new file mode 100644 index 00000000000..463be4faf13 --- /dev/null +++ b/src/utilities/fixes/graphql.ts @@ -0,0 +1,7 @@ +import { undo } from './process'; +import { isType } from 'graphql'; + +export function applyFixes() { + isType(null); + return undo(); +} diff --git a/src/utilities/fixes/index.ts b/src/utilities/fixes/index.ts new file mode 100644 index 00000000000..6362beb7cb9 --- /dev/null +++ b/src/utilities/fixes/index.ts @@ -0,0 +1,3 @@ +export { default as __DEV__ } from "./__DEV__"; +import { applyFixes } from "./graphql"; +applyFixes(); diff --git a/src/utilities/fixes/process.ts b/src/utilities/fixes/process.ts new file mode 100644 index 00000000000..6ee1840bd60 --- /dev/null +++ b/src/utilities/fixes/process.ts @@ -0,0 +1,34 @@ +import { maybe } from "../common/maybe"; +import global from "../common/global"; + +let accessCount = 0; +let needToUndo = false; + +if (global && maybe(() => process.env.NODE_ENV) === void 0) { + const stub = { + env: { + NODE_ENV: "production", + }, + }; + + Object.defineProperty(global, "process", { + get() { + ++accessCount; + return stub; + }, + configurable: true, + enumerable: false, + writable: false, + }); + + // We expect this to be true now. + needToUndo = "process" in global; +} + +export function undo() { + if (needToUndo) { + delete (global as any).process; + needToUndo = false; + } + return accessCount; +} diff --git a/src/utilities/index.ts b/src/utilities/index.ts index b5d8b3a51db..87e28b1e9d3 100644 --- a/src/utilities/index.ts +++ b/src/utilities/index.ts @@ -1,3 +1,5 @@ +export { __DEV__ } from "./fixes"; + export { DirectiveInfo, InclusionDirectives,