Typescript incremental builds using project references, test type checking, better dev workflow

[rosskevin]

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

What it does

This PR improves developer workflow, addresses some maintenance issues and simplifies builds with common configs. It also builds faster using ts incremental builds and adds type checking to all src including tests.

Rationale

I got into the code to look at contributing to another issue, and noticed ts errors in my vscode workspace, specifically the tests. As I dug in, I noticed that tests aren't run against tsc, and the tsconfigs were duplicative and sometimes differing.

Goals

• enable type checking on all src including tests
• simpler tsconfigs
• simpler developer workflow
• parallelized task running
• maintainability and cleanup since joining individual projects together

Changes

IMPORTANT REVIEW NOTE .ts code changes were solely either a) adding any; or b) the result of testing scripts and running prettier or lint:fix on files which hadn't previously been touched. There is no intent here to change any runtime code, only build process and minimal changes to get tests type checking. Using diff options without spaces may help. To simplify getting tests to typecheck, they were left as much as-is as possible. These types can be incrementally improved moving forward.

• use typescript 3.x project references - again simplifies dependencies and work specifically in monorepos with the added benefit of incremental builds
• use es5/common js tsc builds by default - important for ts project references to resolve/build incrementally
• since default builds are now commonjs, added the rollup esm bundle
• validate tests with typescript (remove tests from excludes in tsconfigs)
• upgrade ts-jest - newer versions have type checking
• changes to parallelize tasks loads with lerna making for faster experience on post-install and other tasks
• move prettier settings to .prettierrc to prevent fighting between lint-staged and vscode prettier plugin
• use more common tsconfig base settings
• fix exclusions in tsconfigs
• where ts errors were revealed by the use of common settings, used any for now to move past (effectively the same as before). These should be systematically improved in further PRs.
• use jest multi-project runner
• share jest configs
• share circleci configs and used prerequisite/shared step called Prepare

To be done in different PRs

I want to limit the size of this PR, so I have not continued on with my ultimate goals but will do the following using this PR as the basis:

• enable strict: true to expose more static type issues
• start removing any in tests incrementally

To be done by maintainers

• alter user script and remove redundant npm i in netlify - see Typescript incremental builds using project references, test type checking, better dev workflow #4261 (comment)

[hwillson]

@rosskevin After the failing docs issue is resolved, do you think this PR is ready for a full review?

[rosskevin]

Ready for review. Any build/bundle improvements contemplated can be PR'd separately.

[benjamn]

Let's focus on the bundling changes first, and possibly extract them into a smaller PR that changes fewer things at once.

Here's my current thinking on bundling vs. exploded modules, etc.:

1. The main field of package.json should point to lib/index.js, a CommonJS module (not a bundle) that imports other (exploded) .js files from the lib directory, which correspond 1:1 with .ts files in the src directory. Pointing main to a tree of separate CommonJS modules is what Node expects, and it enables non-tree-shaking bundlers to do basic module pruning more easily than with a single CommonJS bundle. This also means we shouldn't need to build any individual UMD bundles for alternate entry points, since those consumers can just import the CommonJS modules.

2. The module field of package.json should point to a single ESM bundle file (lib/bundle.esm.js), compiled by Rollup. It's important to use a bundle here, instead of exploded ESM modules, because we're using exploded lib/*.js modules for the CommonJS import tree, so it would be tricky to explode ESM modules as well, without renaming the modules or introducing another output directory besides lib. Also, we don't want to encourage folks to use sub-module imports with ESM modules, because sub-module imports ignore the module field of package.json. Fortunately, ESM bundlers like Rollup can do tree-shaking just as effectively against a single bundle.

3. A .d.ts file should exist alongside every CommonJS .js file under the lib directory, and the types field of package.json should point to lib/index.d.ts, which imports/re-exports the other .d.ts files. This should work equally well for CommonJS imports (including sub-module imports) and for ESM imports, because ESM imports will always refer to the top-level package, never sub-modules, so lib/index.d.ts will provide the correct types for both lib/bundle.esm.js and lib/index.js.

4. If we can avoid having a browser field in package.json at all, then we should just rely on the main and module fields, at least one of which will be understood by every bundler. In other words, we should not need to build a lib/bundle.umd.js file. I'm worried that having a browser field that points to a UMD bundle will trick some bundlers into using that bundle, when they could easily just use the main field. The packages in this repository are intended to be consumed primarily by clients, so the main field should always point to browser-safe code, and never anything server-specific.

5. If possible, the src/index.ts module should somehow enforce that it is only ever evaluated in one mode (CommonJS or ESM, but not both). This might be tricky because the code would need to detect the current module system. Maybe Rollup can inject this information by replacing a constant like APOLLO_CLIENT_IS_ESM with different values when it builds lib/index.js and lib/bundle.esm.js? Then we would need to keep track of which version of the code has loaded, using some shared state (possibly global), so we can prevent the other version from loading. I can look into this in a separate PR, if you prefer.

[rosskevin]

1. I think pulling out he UMD from the package.json is a good idea. Do we need it at all for bundlesize checking?
2. Splitting this is a non-starter for me. The ts-test checking and the build/bundling are too intertwined.

I don't have a lot of time today/tomorrow but I'll do a quick pass on this to re-familiarize myself and get back to you within an hour.

[benjamn]

@rosskevin Alternatively, now that we're back from vacation, we can totally pick this up and do some of the heavy lifting to split the PR. We will cherry-pick your commits wherever we can. You've already done a lot of exploration, and we're grateful for that work!

[rosskevin]

• I just pushed the removal of browser from package.json. The umd's are still built for the sole purpose of bundlesize measures. I'm not sure that is the best path going forward but feel free to yank the umd entirely and look at using something like https://github.com/TrySound/rollup-plugin-size-snapshot as a new reference for measures. I'm not sure the best path forward but TrySound is well versed in this area and has been a big help to me with understanding best bundling strategies.
• "main" as-is points to cjs exploded with .d.ts
• "module" points to esm bundle

If possible, the src/index.ts module should somehow enforce that it is only ever evaluated in one mode (CommonJS or ESM, but not both)

I'm not sure what you mean by that. Both the cjs and esm outputs are based on the index.ts.

Alternatively, now that we're back from vacation, we can totally pick this up

Please feel free. I've got a release going out in the morning. Regarding splitting this, I would offer that I think that is a waste of time. This is primarily a build process change and it is exercised via tests
(which are minimally changed). One without the other doesn't make sense to me. I suggest a squash+merge.

Please don't forget, the netlify user script needs altered, it is doing duplicate work (even on master).

[JoviDeCroock]

And don't forget that apollo-boost is still not bundling in its dependencies, haven't had time to look into that yet (busy days).

[rosskevin]

And don't forget that apollo-boost is still not bundling in its dependencies, haven't had time to look into that yet (busy days).

This is related to UMD only, so nothing downstream effected. Switching a size-checking strategy would obsolete it.

[benjamn]

I'm not sure what you mean by that. Both the cjs and esm outputs are based on the index.ts.

Yes, but src/index.ts becomes both lib/index.js and (a small part of) lib/bundle.esm.js, and it's a mistake if both of those two compiled modules are used in the same application. That's the failure case I'd like to guard against.

[rosskevin]

Yes, but src/index.ts becomes both lib/index.js and (a small part of) lib/bundle.esm.js, and it's a mistake if both of those two compiled modules are used in the same application. That's the failure case I'd like to guard against.

There is no guard against this as I detail here and in the link to material-ui-pickers pr. Webpack has indicated there will be no interop indicated here webpack/webpack#7973

The absolute only guard is to use only bundles. That is exactly why I made this recommendation here #4234 (comment).

Summarizing:

My strong recommendation at this point would actually be to only use this format:

1. main -> bundle.cjs.js
2. module -> bundle.esm.js
3. types -> index.d.ts (exploded js files, preferably esm, next to .d.ts files).

Now the above with package.json pointing to only rollup bundles would guarantee a user would not encounter interop problems I mentioned, but then would not provide any path imports either. Given the hidden interop problems, this seems like the best set of artifacts for the majority of users. I'm not sure path imports are a good tradeoff given the hidden downsides and implications to tree shaking.

If exploded paths are referenced by either main or module you have to accept that the user can opt-into interop problems unknowingly by using a combination of index and path imports.

[benjamn]

You could still accidentally load both lib/bundle.cjs.js and lib/bundle.esm.js in the same application, though. I agree there's no way to prevent that from happening, but I think we should at least be able to warn when it happens.

[rosskevin]

You could still accidentally load both lib/bundle.cjs.js and lib/bundle.esm.js in the same application

How would you do that?

Using 80/20 rule which artifact strategy makes most sense for the most users?

[benjamn]

Here's the narrative that makes sense to me:

If you're concerned about bundle sizes, make sure you're loading all Apollo Client packages using the module field in package.json. If you're using the main field, everything will work, but we can't guarantee your bundle will be as small as possible.

In other words, I completely agree that the ESM entry point should be a single bundle, but I think it's fine if the main entry point is an exploded tree of CommonJS modules.

I don't think we have to hold people's hands to make sure they only use the module entry point, but it would be nice to help them be 100% sure they've gotten it right.

[benjamn]

Closing in favor of #4401, which includes many of these changes. Thanks for doing all this work @rosskevin!