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 4, 2021
1 parent 1f804a5 commit 023a824
Show file tree
Hide file tree
Showing 24 changed files with 154 additions and 142 deletions.
5 changes: 4 additions & 1 deletion CHANGELOG.md
Expand Up @@ -20,11 +20,14 @@
- Make sure the `MockedResponse` `ResultFunction` type is re-exported. <br/>
[@hwillson](https://github.com/hwillson) in [#8315](https://github.com/apollographql/apollo-client/pull/8315)

### Potentially breaking changes
### Potentially disruptive changes

- To avoid retaining sensitive information from mutation root field arguments, Apollo Client v3.4 automatically clears any `ROOT_MUTATION` fields from the cache after each mutation finishes. If you need this information to remain in the cache, you can prevent the removal by passing the `keepRootFields: true` option to `client.mutate`. `ROOT_MUTATION` result data are also passed to the mutation `update` function, so we recommend obtaining the results that way, rather than using `keepRootFields: true`, if possible. <br/>
[@benjamn](https://github.com/benjamn) in [#8280](https://github.com/apollographql/apollo-client/pull/8280)

- Internally, Apollo Client now controls the execution of development-only code using the `__DEV__` global variable, rather than `process.env.NODE_ENV`. While this change should not cause any visible differences in behavior, it will increase your minified+gzip bundle size by more than 3.5kB, unless you configure your minifier to replace `__DEV__` with a `true` or `false` constant, the same way you already replace `process.env.NODE_ENV` with a string literal like `"development"` or `"production"`. For an example of configuring a Create React App project without ejecting, see this pull request for our [React Apollo reproduction template](https://github.com/apollographql/react-apollo-error-template/pull/51). <br/>
[@benjamn](https://github.com/benjamn) in [#8347](https://github.com/apollographql/apollo-client/pull/8347)

- Internally, Apollo Client now uses namespace syntax (e.g. `import * as React from "react"`) for imports whose types are re-exported (and thus may appear in `.d.ts` files). This change should remove any need to configure `esModuleInterop` or `allowSyntheticDefaultImports` in `tsconfig.json`, but might require updating bundler configurations that specify named exports of the `react` and `prop-types` packages, to include exports like `createContext` and `createElement` ([example](https://github.com/apollographql/apollo-client/commit/16b08e1af9ba9934041298496e167aafb128c15d)). <br/>
[@devrelm](https://github.com/devrelm) in [#7742](https://github.com/apollographql/apollo-client/pull/7742)

Expand Down
45 changes: 17 additions & 28 deletions config/processInvariants.ts
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
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
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
@@ -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
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
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
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
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
Expand Up @@ -151,7 +151,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 @@ -205,7 +205,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
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
Expand Up @@ -802,7 +802,7 @@ export class QueryManager<TStore> {
});
}

if (process.env.NODE_ENV !== "production" && queryNamesAndDocs.size) {
if (__DEV__ && queryNamesAndDocs.size) {
queryNamesAndDocs.forEach((included, nameOrDoc) => {
if (!included) {
invariant.warn(`Unknown query ${
Expand Down Expand Up @@ -1364,7 +1364,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
2 changes: 2 additions & 0 deletions src/core/index.ts
@@ -1,5 +1,7 @@
/* Core */

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

export {
ApolloClient,
ApolloClientOptions,
Expand Down
2 changes: 2 additions & 0 deletions src/react/index.ts
@@ -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.

0 comments on commit 023a824

Please sign in to comment.