Rework interdependencies between @apollo/client/* entry points. #6656

benjamn · 2020-07-20T18:31:39Z

Now that we've added a bunch of new entry points for importing specific parts of Apollo Client (e.g. @apollo/client/react/* in #5357 and #6558 and @apollo/client/link/* in #6577), it's important to consider how they all relate to each other.

If we let Rollup do its thing, any shared code imported by multiple different entry point bundles will end up duplicated across the bundles, which is always a bundle size concern, and often a threat to correctness, especially in cases where important classes and functions are assumed to be singletons.

Note: these problems primarily affect the CommonJS bundles (dist/**/*.cjs.js) built by Rollup from TypeScript-compiled ESM modules (and consumed primarily by Node.js), and are less of a threat for the ESM modules, since the structure of the dependency graph is preserved by the ESM modules, allowing most bundlers to avoid duplication of modules when bundling ESM code.

To fix these problems once and for all, we need to teach Rollup that importing from a different entry point counts as an external import, so Rollup will not bundle the imported code into the current bundle, or try to rewrite the imported module identifier in any way.

Then, we need to avoid any nested imports that reach inside other entry points, and instead enforce that any cross-entry-point imports import only from the entry point directory, which must have a package.json with both main and module fields, along with an index.ts file that defines the exports of the entry point. These kinds of changes account for most of the lines modified by this PR. If an entry point doesn't export something, it's not okay to reach inside the entry point and pull out the thing you want, because that leads to duplication. Instead, the entry point's index.ts file will need to explicitly export anything needed by code outside of the entry point.

Finally, to minimize the risk that this PR will introduce breaking changes, we can use snapshot testing to verify that the set of exports for each entry point has not changed in unexpected ways. I'm very happy with how this turned out (see the very first commit, and the final few commits, specifically).

There's a lot here, but I recommend reviewing commit-by-commit. You can breeze through the Fix cross-entry-point imports in ... commits once you get the hang of them, but the others are pretty important.

Before we get started refactoring how entry points are built, let's take a snapshot of the current exports of all public entry points.

This new module provides a single source of truth for all the nested entry points that we build. As a nice bonus, thanks to this unification, the @apollo/client/testing entry point no longer needs such special treatment.

This prevents collision between the actual main entry point (used by Node.js) and the dist/apollo-client.cjs.js bundle that we use for estimating the minified bundle size.

Adapted from https://github.com/apollographql/invariant-packages/tree/master/packages/rollup-plugin-invariant, this script wraps invariant(...), new InvariantError(...), and invariant.{warn,error} calls with process.env.NODE_ENV logic, so that long error messages can be stripped from production builds. I decided to abandon the Rollup plugin for this task because I'm planning to change how Rollup determines if imported modules are "external," and Rollup (understandably) stops traversing the dependency graph when it hits an external import, which means we can no longer rely on Rollup to traverse all modules that need invariants to be transformed. Also, this transformation step was never especially intertwined with the dependency traversal and rewriting logic that Rollup provides, so I think it makes things simpler to keep them separate. If Rollup provided hooks for performing arbitrary build steps, we might have been able to do this work in some initial step, but I couldn't find anything in the Rollup documentation that fit the bill (and that's fine: I wouldn't expect Rollup to attempt to be a generic build pipeline). Now, the CJS bundles for each nested entry point (e.g. @apollo/client/core or @apollo/client/react/ssr) can be computed from code that has been processed by the invariant transform, but has not yet been tampered with by Rollup, so we can trust that the exact same module identifiers from the source code are still intact.

There's not currently any risk of running the invariant transform over the ./dist directory more than once during our build process, but it doesn't hurt to make sure the transform is idempotent.

Since we aren't using Rollup to apply the invariant transform anymore, it turns out we don't need this build step at all. Instead, the CJS bundles will be generated directly from the modules generated by tsc and processed by the invariant transform.

This is a vital change (the whole point of this PR, really), because it means Rollup will no longer attempt to include code from other entry points in the CJS bundles for unrelated entry points, so there will no longer be any duplication of code between the CJS bundles.

It would be nice to make this an error that would fail the build, but there are a couple of benign violations that would be disruptive to fix at this point, like reexporting `../ApolloClient` from `./core/QueryManager`, and `./testing/index.ts` reaching into `../utilities/testing`.

Now that the CommonJS module tree uses nested entry points with their own package.json files, instead of importing everything and then filtering it (as the CJS version of @apollo/client/core used to do), we do not need to be so paranoid about the possibility that React is not installed.

benjamn · 2020-07-20T18:45:50Z

src/__tests__/exports.ts

+describe('exports of public entry points', () => {
+  const testedIds = new Set<string>();
+
+  function check(id: string, ns: Namespace) {
+    it(id, () => {
+      testedIds.add(id);
+      expect(Object.keys(ns).sort()).toMatchSnapshot();
+    });
+  }
+
+  check("@apollo/client", client);
+  check("@apollo/client/cache", cache);
+  check("@apollo/client/core", core);


It would be ideal to check the CJS exports using require (as well as ESM exports, which this test is doing), but Jest doesn't run the build first, and it compiles code directly from ./src rather than looking inside ./dist (which is where all the CJS code ends up).

benjamn · 2020-07-20T18:47:04Z

src/cache/inmemory/inMemoryCache.ts

-import { addTypenameToDocument } from '../../utilities/graphql/transform';
-import { StoreObject, Reference, isReference }  from '../../utilities/graphql/storeUtils';
+import {
+  addTypenameToDocument,
+  StoreObject,
+  Reference,
+  isReference,
+} from '../../utilities';


Importing from the entry point directory (rather than reaching inside of it) has the nice side-benefit of allowing multiple import statements to be merged into one, in many cases.

benjamn · 2020-07-20T18:49:00Z

src/testing/index.ts

@@ -0,0 +1 @@
+export * from '../utilities/testing';


I would love to ditch this module and just move src/utilities/testing to src/testing, but that would have required a lot of changes to imports across our test suite, so it seemed potentially disruptive.

Here's an example of one of the warnings from f634785 that this code causes:

dist/testing/index.js → dist/testing/testing.cjs.js... Risky cross-entry-point nested import of ../utilities/testing in testing/index.js created dist/testing/testing.cjs.js in 24ms

The problem is that ../utilities is an entry point directory, so importing something from inside it is invasive. In this particular case, it's mostly fine because the @apollo/client/utilities doesn't use or export anything from the utilities/testing directory.

hwillson

This is incredible @benjamn, and really helped cleanup the build process / imports as a nice side effect. Thanks very much for working through all of this!

hwillson · 2020-07-20T19:09:03Z

CHANGELOG.md

@@ -1,3 +1,10 @@
+## Apollo Client 3.1.0


I'm 👍 on targeting 3.1.0 for this.

hwillson · 2020-07-20T19:09:19Z

config/entryPoints.js

+const entryPoints = [
+  { dirs: [], bundleName: "main" },
+  { dirs: ['cache'] },
+  { dirs: ['core'] },
+  { dirs: ['errors'] },
+  { dirs: ['link', 'batch'] },
+  { dirs: ['link', 'batch-http'] },
+  { dirs: ['link', 'context'] },
+  { dirs: ['link', 'core'] },
+  { dirs: ['link', 'error'] },
+  { dirs: ['link', 'http'] },
+  { dirs: ['link', 'retry'] },
+  { dirs: ['link', 'schema'] },
+  { dirs: ['link', 'utils'] },
+  { dirs: ['link', 'ws'] },
+  { dirs: ['react'] },
+  { dirs: ['react', 'components'] },
+  { dirs: ['react', 'context'] },
+  { dirs: ['react', 'data'] },
+  { dirs: ['react', 'hoc'] },
+  { dirs: ['react', 'hooks'] },
+  { dirs: ['react', 'parser'] },
+  { dirs: ['react', 'ssr'] },
+  { dirs: ['utilities'] },
+  { dirs: ['testing'], extensions: [".js", ".jsx"] },
+];


I love this approach! 🎉

hwillson · 2020-07-20T19:10:10Z

src/__tests__/exports.ts

+  it("completeness", () => {
+    const { join } = require("path").posix;
+    entryPoints.forEach((info: Record<string, any>) => {
+      const id = join("@apollo/client", ...info.dirs);
+      expect(testedIds).toContain(id);
+    });
+  });


Thanks for adding this - I would have been the first one to forget! 😂

This partially reverts commit 627bd1b. PR #6656 removed a function called `prepareESM`, which was no longer needed for the sake of running `rollup-plugin-invariant`, but which had another benefit: Rollup would replace imports/exports like // Example exports from ./dist/index.js: export * from './core' export * from './react' with their fully-expanded (and file-extensioned) equivalents: ... export { makeVar } from './cache/inmemory/reactiveVars.js'; export { defaultDataIdFromObject } from './cache/inmemory/policies.js'; export { InMemoryCache } from './cache/inmemory/inMemoryCache.js'; export { ApolloClient } from './core/ApolloClient.js'; ... Although this may look like more code, it's effectively what the bundler/browser will have to do anyway, and including the `.js` file extensions makes this code a little more browser- and Rollup-friendly (that is, you shouldn't have to use @rollup/plugin-node-resolve to bundle @apollo/client with Rollup). The expanded imports/exports are also closer to the ESM code shipped with @apollo/client@3.0.x, so restoring this behavior helps with the goal of keeping the changes in #6656 backwards-compatible for v3.1.0. Note that the CommonJS bundles used by Node.js are built before this step, so this expansion of imports does not affect Node.js.

This partially reverts commit 627bd1b from PR #6656. PR #6656 removed a function called `prepareESM`, which was no longer needed for the sake of running `rollup-plugin-invariant`, but which had another benefit: Rollup would replace imports/exports like // Example exports from ./dist/index.js: export * from './core' export * from './react' with their fully-expanded (and file-extensioned) equivalents: ... export { makeVar } from './cache/inmemory/reactiveVars.js'; export { defaultDataIdFromObject } from './cache/inmemory/policies.js'; export { InMemoryCache } from './cache/inmemory/inMemoryCache.js'; export { ApolloClient } from './core/ApolloClient.js'; ... Although this may look like more code, it effectively saves the bundler/browser work by reducing indirection, and including the .js file extensions makes this code a little more browser- and Rollup-friendly (that is, you shouldn't have to use @rollup/plugin-node-resolve to bundle @apollo/client with Rollup). The expanded imports/exports are also closer to the ESM code shipped with @apollo/client@3.0.x, so restoring this behavior helps with the goal of keeping the changes in #6656 backwards-compatible for v3.1.0. Note that the CommonJS bundles used by Node.js are built before this step, so this expansion of imports does not affect Node.js.

…6657) This partially reverts commit 627bd1b from PR apollographql#6656. PR apollographql#6656 removed a function called `prepareESM`, which was no longer needed for the sake of running `rollup-plugin-invariant`, but which had another benefit: Rollup would replace imports/exports like // Example exports from ./dist/index.js: export * from './core' export * from './react' with their fully-expanded (and file-extensioned) equivalents: ... export { makeVar } from './cache/inmemory/reactiveVars.js'; export { defaultDataIdFromObject } from './cache/inmemory/policies.js'; export { InMemoryCache } from './cache/inmemory/inMemoryCache.js'; export { ApolloClient } from './core/ApolloClient.js'; ... Although this may look like more code, it effectively saves the bundler/browser work by reducing indirection, and including the .js file extensions makes this code a little more browser- and Rollup-friendly (that is, you shouldn't have to use @rollup/plugin-node-resolve to bundle @apollo/client with Rollup). The expanded imports/exports are also closer to the ESM code shipped with @apollo/client@3.0.x, so restoring this behavior helps with the goal of keeping the changes in apollographql#6656 backwards-compatible for v3.1.0. Note that the CommonJS bundles used by Node.js are built before this step, so this expansion of imports does not affect Node.js.

…6656 link.

These types were lost in #6656 when I switched from export * from '../cache' to using named exports. There are some @apollo/client/cache exports that I wanted to omit from @apollo/client/core (like cacheSlot), but I definitely did not intend to omit these types. Fixes #6723.

benjamn added 30 commits July 20, 2020 13:37

Snapshot-test the exported properties of each entry point.

442923b

Before we get started refactoring how entry points are built, let's take a snapshot of the current exports of all public entry points.

Simplify buildPackageJson in config/prepareDist.js.

38ed28f

Align prepareBundle list with buildPackageJson list.

d064c75

Import entry points from separate config/entryPoints.js module.

847a4a6

This new module provides a single source of truth for all the nested entry points that we build. As a nice bonus, thanks to this unification, the @apollo/client/testing entry point no longer needs such special treatment.

Rename main CommonJS entry point to dist/main.cjs.js.

0121fc6

This prevents collision between the actual main entry point (used by Node.js) and the dist/apollo-client.cjs.js bundle that we use for estimating the minified bundle size.

Replace rollup-plugin-node-resolve with @rollup/plugin-node-resolve.

30ccd41

Avoid re-processing invariant expressions.

a075a2a

There's not currently any risk of running the invariant transform over the ./dist directory more than once during our build process, but it doesn't hurt to make sure the transform is idempotent.

Process invariants in build step, and remove invariantPlugin.

687eaa3

Make @apollo/client/errors an entry point.

44b5415

Make @apollo/client/react/hooks an entry point.

45bf62a

Make @apollo/client/react/context an entry point.

d654457

Make @apollo/client/react/data an entry point.

a698c54

Make @apollo/client/react/parser an entry point.

1934b98

Make @apollo/client/link/utils an entry point.

112e477

Explicitly prepare dist/apollo-client.cjs.js for bundlesize.

4e1c604

Fix cross-entry-point imports in src/utilities.

ca230cd

Fix cross-entry-point imports within src/cache.

8388284

Fix cross-entry-point imports within src/core.

2e592b4

Fix cross-entry-point imports in src/ApollClient.ts.

dc403e1

Fix cross-entry-point imports in src/react/data.

8363f90

Fix cross-entry-point imports in src/react/hooks.

02ef623

Fix cross-entry-point imports in src/react/hoc.

241d596

Fix cross-entry-point imports in src/react/components.

aefbc38

Fix cross-entry-point imports in src/react/context.

94becda

Fix cross-entry-point imports in src/react/ssr.

2c30f3d

benjamn commented Jul 20, 2020

View reviewed changes

Note PR #6656 in CHANGELOG.md, targeting v3.1.0.

43fc12d

benjamn mentioned this pull request Jul 20, 2020

setContext not being called for query #6623

Closed

hwillson approved these changes Jul 20, 2020

View reviewed changes

benjamn mentioned this pull request Jul 20, 2020

Export Observable from @apollo/client/utilities #6642

Closed

benjamn merged commit 7993a77 into master Jul 20, 2020

benjamn mentioned this pull request Jul 20, 2020

Process ESM .js modules in ./dist with Rollup, again. #6657

Merged

benjamn added a commit that referenced this pull request Jul 21, 2020

Mention PR #6657 in CHANGELOG.md, and fix #6656 link.

f60ee39

benjamn deleted the revamp-commonjs-interdependencies branch July 21, 2020 14:48

This was referenced Jul 21, 2020

SyntaxError: Unexpected token 'export' v 3.0.2 #6666

Closed

Integrate apollo-upload-client in @apollo/client package #6671

Closed

Integrate apollo-link-logger in @apollo/client package #6667

Closed

jimrandomh pushed a commit to jimrandomh/apollo-client that referenced this pull request Jul 22, 2020

Mention PR apollographql#6657 in CHANGELOG.md, and fix apollographql#…

e4a24ea

…6656 link.

benjamn mentioned this pull request Jul 23, 2020

Support async render function in getDataFromTree / getMarkupFromTree #6576

Merged

2 tasks

JoshRobertson mentioned this pull request Jul 28, 2020

DataProxy and NormalizedCacheObject types not available from @apollo/client in 3.1 without deep import #6723

Closed

benjamn mentioned this pull request Jul 28, 2020

Re-export cache types from @apollo/client/core, again. #6725

Merged

benjamn mentioned this pull request Jul 29, 2020

How should I import WebSocketLink ? #6732

Closed

benjamn mentioned this pull request Feb 2, 2021

Unable to use files in cache/core and cache/inmemory in a node environment #7639

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework interdependencies between @apollo/client/* entry points. #6656

Rework interdependencies between @apollo/client/* entry points. #6656

benjamn commented Jul 20, 2020 •

edited

Loading

benjamn Jul 20, 2020

benjamn Jul 20, 2020

benjamn Jul 20, 2020

benjamn Jul 20, 2020

hwillson left a comment

hwillson Jul 20, 2020

hwillson Jul 20, 2020

hwillson Jul 20, 2020

Rework interdependencies between @apollo/client/* entry points. #6656

Rework interdependencies between @apollo/client/* entry points. #6656

Conversation

benjamn commented Jul 20, 2020 • edited Loading

benjamn Jul 20, 2020

Choose a reason for hiding this comment

benjamn Jul 20, 2020

Choose a reason for hiding this comment

benjamn Jul 20, 2020

Choose a reason for hiding this comment

benjamn Jul 20, 2020

Choose a reason for hiding this comment

hwillson left a comment

Choose a reason for hiding this comment

hwillson Jul 20, 2020

Choose a reason for hiding this comment

hwillson Jul 20, 2020

Choose a reason for hiding this comment

hwillson Jul 20, 2020

Choose a reason for hiding this comment

benjamn commented Jul 20, 2020 •

edited

Loading