Use tsc for builds #1403

robknight · 2024-01-10T10:46:52Z

This PR supersedes #1398 and includes many of the same changes, with some additions.

As with the previous PR, we now have faster builds, resilience against missing/frequently changing type declaration files, and lower overall memory usage during development mode.

A summary of the final changes is below:

Replace `tsup` with `tsc`

Previously we used tsup to transpile our TypeScript to JavaScript, with tsc used only for generating types. tsup has some advantages, and in particular does not require as much configuration as tsc to produce the kinds of outputs that we want. Since we now have extensive configuration for tsc, this advantage is diminished.

In particular, getting tsup to build both CJS and ESM variants of JavaScript is a simple matter of passing --format cjs,esm to tsup, whereas tsc can, by default, only build a single variant, according to how tsconfig.json is set up. This low barrier to initial setup made tsup attractive.

tsup also has some features for dealing with polyfills and node-specific builds. We made only minor use of these, in the @pcd/passport-crypto and @pcd/util packages, and it was possible to find simple solutions to the problems in those packages without using tsup.

cjs/esm configuration and simplification

Although tsup doesn't require config files, it does take configuration on the command line. This meant that some of our packages had several entries in package.json to cover the different variants. For example:

    "build": "yarn build:browser && yarn build:node && yarn build:types",
    "build:browser": "tsup src/index.ts --platform=browser --out-dir=./dist/browser --format esm --clean",
    "build:node": "tsup src/index.ts --platform=node --out-dir=./dist/node --format cjs,esm --clean",
    "build:types": "rm -rf dist/types && tsc --emitDeclarationOnly --outDir dist/types",

Is now replaced by:

    "build": "tsc -b tsconfig.cjs.json tsconfig.esm.json",

This single command transpiles both JavaScript variants, and generates type declarations.

The down-side is that we require tsconfig.esm.json and tsconfig.cjs.json to exist. These are both fairly small files, and have an identical template in all packages:

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "outDir": "dist/esm",
    "module": "ESNext",
    "tsBuildInfoFile": "./tsconfig.esm.tsbuildinfo"
  },
  "include": ["src", "src/*.json"]
}

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "outDir": "dist/cjs",
    "module": "CommonJS",
    "tsBuildInfoFile": "./tsconfig.cjs.tsbuildinfo"
  },
  "include": ["src", "src/*.json"]
}

The differing outDir settings tell TypeScript where to put the transpiled JavaScript, the module setting tells is which variant of JavaScript to produce, and tsBuildInfoFile tells it where to put the incremental build data. This setting is important as by default it would be placed alongside the exported JavaScript, meaning that it would get included in the package uploaded to NPM. Since these files are often quite large, it's important to avoid this.

TypeScript references

TypeScript references are entries that optionally appear in the tsconfig.*.json files when a package depends on another package. These allow the TypeScript compiler to build a dependency graph and build types for each package in the right order. This can be used both for the in-memory representation of the types used by the TypeScript language server (and therefore by VSCode), and helps to keep the representation of the types up-to-date irrespective of the on-disk type declaration files.

This can also be used to drive the transpilation and generation of type declaration files. At the root of the project there are now two files called tsconfig.cjs.json and tsconfig.esm.json respectively. These do nothing other than hold references to all of the TypeScript packages in packages/*. Running tsc -b tsconfig.cjs.json tsconfig.esm.json at the root of the repository transpiles all of the packages and generates all of the type declarations in one go. This is much faster than the previous approach of spawning tsup processes per-package and running tsc on a per-package basis to generate types. It is sufficiently fast that it can be run in response to code changes, and is suitable for development mode.

This means that all rebuilds, whether development mode or production builds, include type-checking.

It is important that these references are kept up-to-date, and they may need to be updated whenever we:

Add a dependency between packages
Remove a dependency between packages
Add a new package
Remove an existing package

Fortunately there is a script to update the references. Just run yarn fix-references and it will automatically update the tsconfig files and reformat them using prettier.

Other fixes

Polyfills

In a couple of places (@pcd/util and @pcd/passport-crypto) we did some dynamic require calls to load the node crypto module. This is tricky to transpile because require isn't supported in ESM modules. I've removed this code and replaced it with a dependency on the get-random-values package, which handles the selection of the appropriate crypto engine for us.

A similar problem occurs with the usage of buffer without a dependency on the buffer npm package for browser use. Adding an explicit dependency solves this. In general, polyfilling should be avoided in our packages, and left to the bundler used by the application that depends on the package (in our case, esbuild for passport-client).

Icons

Originally passport-client had a set of SVG icons that were loaded using an esbuild plugin. Because it became necessary to depend on these icons in the PCD UI packages, I moved them to passport-ui and used the equivalent tsup plugin. However, since we don't have tsup now, we needed another solution.

The tsup plugin was just converting the SVG files to dataurl strings. I've written a simple script which loads the SVG files, converts them to data URLs, and then writes those as strings in a .ts file. This produces the same exported structure as previously, without the use of the plugin. It does mean that if we add or change the icons, we will need to run the script again.

Template

To make it easier to create new packages, I've created a template package at templates/package. This includes all of the basic files required for a package. When creating new packages, we should copy and modify this rather than copying an existing package - this will help to avoid accidentally copying some unwanted configuration or dependencies.

Build vs dev

The yarn build command runs yarn build for each package and application individually. Turborepo caching is currently disabled. However, in our base package tsconfig I have enabled composite: true, which enables incremental builds for TypeScript. This means that most of our packages don't need a full rebuild, making builds reasonably fast (though still not as fast as they were with Turborepo caching).

yarn dev runs scripts/watch.ts, which watches the TypeScript source and runs tsc -b on the root tsconfig files, incrementally recompiling any packages that need it. This seems reasonably fast to me. I did not use tsc --watch, since the benefit of doing so seemed minimal and the visual output it produces is a bit confusing.

Dev mode for passport-server and consumer-server now uses ts-node-dev, which will typecheck the application code on startup, and auto-restarts when changes to the application code or packages are observed.
Dev mode for consumer-server now uses ts-node-dev, which type-checks application code on startup and auto-restarts when the application or dependent packages change. I tried using this with passport-server and it doesn't quite work, because passport-server's MultiProcessService relies on a transpiled JavaScript worker.js file existing at a specific path. This means that we need to do a full build of passport-server on every reset to ensure that the file exists.

ichub

Running tsc -b tsconfig.cjs.json tsconfig.esm.json at the root of the repository transpiles all of the packages and generates all of the type declarations in one go. This is much faster than the previous approach of spawning tsup processes per-package and running tsc on a per-package basis to generate types. It is sufficiently fast that it can be run in response to code changes, and is suitable for development mode.

This means that all rebuilds, whether development mode or production builds, include type-checking.

Super cool!

I've removed this code and replaced it with a dependency on the get-random-values package, which handles the selection of the appropriate crypto engine for us.

I'm not sure I like this solution. (I also really appreciate this detailed PR description, it lets me give feedback on stuff like this which I could have missed the opportunity to think about from a bird's eye perspective had it not been documented) I think I would be more comfortable with a package whose purpose/name is one or two more levels general. E.g. it would be able to multiplex different dependencies/functions-in-a-dependency, and not just one function in one particular dependency. E.g. this one package could be responsible for supplying crypto.getRandomValues, crypto2.whatever, another-annoying-dependency.something, and could be itself be named something like dep-multiplexer. Ideally this package would have as little business logic as possible, and be responsible solely for selecting the right thing.

BTW - have you seen this solution to the problem implemented by @Ceedor a few months back?

In general, polyfilling should be avoided in our packages, and left to the bundler used by the application that depends on the package (in our case, esbuild for passport-client).

Could you explain why? Would be useful to have an answer off the top of our head for casses where we have to walk someone through working with our dependencies.

The tsup plugin was just converting the SVG files to dataurl strings. I've written a simple script which loads the SVG files, converts them to data URLs, and then writes those as strings in a .ts file. This produces the same exported structure as previously, without the use of the plugin. It does mean that if we add or change the icons, we will need to run the script again.

Is this the best solution? I'd personally prefer to have as much of the build process automated. If an automated solution is not possible at this time, I'd like to have a github issue that tracks this suggestion.

To make it easier to create new packages, I've created a template package at templates/package. This includes all of the basic files required for a package. When creating new packages, we should copy and modify this rather than copying an existing package - this will help to avoid accidentally copying some unwanted configuration or dependencies.

Amazing! Thanks.

The yarn build command runs yarn build for each package and application individually. Turborepo caching is currently disabled. However, in our base package tsconfig I have enabled composite: true, which enables incremental builds for TypeScript. This means that most of our packages don't need a full rebuild, making builds reasonably fast (though still not as fast as they were with Turborepo caching).

So unfortunate how much of a game of whack-a-mole this process is! If this isn't simple to fix in this PR, let's create an issue. Thanks for being responsive to suggestions from us.

Dev mode for consumer-server now uses ts-node-dev, which type-checks application code on startup and auto-restarts when the application or dependent packages change. I tried using this with passport-server and it doesn't quite work, because passport-server's MultiProcessService relies on a transpiled JavaScript worker.js file existing at a specific path. This means that we need to do a full build of passport-server on every reset to ensure that the file exists.

OK for now 👍

artwyman · 2024-01-10T20:42:57Z

Thanks for all of the investigation, experimentation and careful effort behind this. I think there's enough detail here that I'm going to bow out of reviewing in detail. Ivan's going to do so anyway, and I don't really know any more about a lot of the tradeoffs here than what you've told me.

I'll only comment on the PR description (which is great and detailed). The details all sound reasonable to me with a few bits of feedback:

Losing Turborepo caching seems unfortunate, so I agree should have an Issue to revisit later.
Does the template package actually get built? That seems important to ensure it stays as up-to-date as possible for future tooling changes.
Can yarn fix-references be checked in CI so that we can't forget to run it? E.g. a CI rule which runs it and fails the build if the output is different from what's checked in. Alternately we could run it automatically in a git hook or an on-save action in VSCode, but those might be more risky.

robknight · 2024-01-10T20:50:29Z

Thanks for all of the investigation, experimentation and careful effort behind this. I think there's enough detail here that I'm going to bow out of reviewing in detail. Ivan's going to do so anyway, and I don't really know any more about a lot of the tradeoffs here than what you've told me.

I'll only comment on the PR description (which is great and detailed). The details all sound reasonable to me with a few bits of feedback:
* Losing Turborepo caching seems unfortunate, so I agree should have an Issue to revisit later.

We still have it for linting and tests, and with incremental builds the build time is not much slower than it was with the cache enabled, so we can probably live without it.

* Does the template package actually get built?  That seems important to ensure it stays as up-to-date as possible for future tooling changes.

No, it doesn't. I'll think about the best way to do this.

* Can `yarn fix-references` be checked in CI so that we can't forget to run it?  E.g. a CI rule which runs it and fails the build if the output is different from what's checked in.  Alternately we could run it automatically in a git hook or an on-save action in VSCode, but those might be more risky.

It is checked in CI!

robknight · 2024-01-10T21:17:47Z

I've removed this code and replaced it with a dependency on the get-random-values package, which handles the selection of the appropriate crypto engine for us.

I'm not sure I like this solution. (I also really appreciate this detailed PR description, it lets me give feedback on stuff like this which I could have missed the opportunity to think about from a bird's eye perspective had it not been documented) I think I would be more comfortable with a package whose purpose/name is one or two more levels general. E.g. it would be able to multiplex different dependencies/functions-in-a-dependency, and not just one function in one particular dependency. E.g. this one package could be responsible for supplying crypto.getRandomValues, crypto2.whatever, another-annoying-dependency.something, and could be itself be named something like dep-multiplexer. Ideally this package would have as little business logic as possible, and be responsible solely for selecting the right thing.

BTW - have you seen this solution to the problem implemented by @Ceedor a few months back?

Yes, but this code causes this:

anon-message-client:build: Failed to compile.
anon-message-client:build:
anon-message-client:build: node:crypto
anon-message-client:build: Module build failed: UnhandledSchemeError: Reading from "node:crypto" is not handled by plugins (Unhandled scheme).
anon-message-client:build: Webpack supports "data:" and "file:" URIs by default.
anon-message-client:build: You may need an additional plugin to handle "node:" URIs.
anon-message-client:build:     at /Users/robknight/Projects/zupass/node_modules/next/dist/compiled/webpack/bundle5.js:28:399825
anon-message-client:build:     at Hook.eval [as callAsync] (eval at create (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/webpack/bundle5.js:13:28867), <anonymous>:6:1)
anon-message-client:build:     at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/webpack/bundle5.js:13:26021)
anon-message-client:build:     at Object.processResource (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/webpack/bundle5.js:28:399750)
anon-message-client:build:     at processResource (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/loader-runner/LoaderRunner.js:1:5308)
anon-message-client:build:     at iteratePitchingLoaders (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/loader-runner/LoaderRunner.js:1:4667)
anon-message-client:build:     at runLoaders (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/loader-runner/LoaderRunner.js:1:8590)
anon-message-client:build:     at NormalModule._doBuild (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/webpack/bundle5.js:28:399612)
anon-message-client:build:     at NormalModule.build (/Users/robknight/Projects/zupass/node_modules/next/dist/compiled/webpack/bundle5.js:28:401640)
anon-message-client:build:     at /Users/robknight/Projects/zupass/node_modules/next/dist/compiled/webpack/bundle5.js:28:82059
anon-message-client:build:
anon-message-client:build: Import trace for requested module:
anon-message-client:build: node:crypto
anon-message-client:build: ../../packages/lib/util/dist/esm/src/CryptoHelpers.js
anon-message-client:build: ../../packages/lib/util/dist/esm/src/index.js

The cause seems to be the require("node:crypto"). This line suspiciously requires eslint to be disabled. Changing the "node:crypto" to "crypto" seems to make it work, but I'm suspicious of dynamic require() calls.

That said, I can put it back the way it was, with "node:crypto" replaced by "crypto" and everything seems to work.

In general, polyfilling should be avoided in our packages, and left to the bundler used by the application that depends on the package (in our case, esbuild for passport-client).

Could you explain why? Would be useful to have an answer off the top of our head for casses where we have to walk someone through working with our dependencies.

The problem is that third party apps will often do their own polyfilling, using their own bundler, and won't know anything about the choices we have made. I think it's easier to stick to writing clean non-environment-specific TypeScript as much as possible, and if we must have code that works across multiple environments, do so by importing battle-tested packages that do this for us.

The tsup plugin was just converting the SVG files to dataurl strings. I've written a simple script which loads the SVG files, converts them to data URLs, and then writes those as strings in a .ts file. This produces the same exported structure as previously, without the use of the plugin. It does mean that if we add or change the icons, we will need to run the script again.

Is this the best solution? I'd personally prefer to have as much of the build process automated. If an automated solution is not possible at this time, I'd like to have a github issue that tracks this suggestion.

I could add this to the build command for the package.

ichub

what is your testing plan for this? things off the top of my head that would be good to make sure works now and potentially set up a system for so we're sure it keeps working into the future (that last part not applicable to every item in the list, only some:

does this continue to work in render?
do packages publish and get consumed properly outside the monorepo? could deploy an alpha version to check.
do the example apps compile and run properly after this change?
when running yarn dev, do changes to any package/app properly cause builds to propagate until the app is restarted?
does this work on a fresh clone of the repo?
would you want me or @artwyman to test anything in particular here?

packages/pcd/webauthn-pcd/package.json

scripts/watch.ts

templates/package/package.json

tsconfig.cjs.json

scripts/fix-references.sh

packages/lib/passport-ui/convert-icons.ts

packages/lib/passport-ui/src/index.ts

packages/pcd/eddsa-frog-pcd/src/EdDSAFrogPCD.ts

robknight · 2024-01-15T11:41:09Z

what is your testing plan for this? things off the top of my head that would be good to make sure works now and potentially set up a system for so we're sure it keeps working into the future (that last part not applicable to every item in the list, only some:
* does this continue to work in render?

This could be tested either in staging or in one the personal staging areas which build off a separate branch. With build caching disabled, the non-determinacy we encountered in staging should not be a factor.

* do packages publish and get consumed properly outside the monorepo? could deploy an alpha version to check.

* do the example apps compile and run properly after this change?

There is the test-packaging.sh script for this, which verifies that:

Packaging works
Packages can be published to a (local) npm repo
Packages can be installed into a each of the examples projects
Those projects still build successfully

We could publish them to the real npm as alpha versions, but I don't think that this would do anything not covered above.

* when running `yarn dev`, do changes to any package/app properly cause builds to propagate until the app is restarted?

In my observation, yes: passport-server uses nodemon for this, and consumer-server uses ts-node-dev --respawn, both of which watch for changes in their dependencies. Restarts occur after the dependencies have been rebuilt in response to changes.

* does this work on a fresh clone of the repo?

I've tested it as working on a fresh clone of the repo. CI builds also use fresh clones and run yarn build/lint/test, though not yarn dev.

* would you want me or @artwyman to test anything in particular here?

The main thing is that yarn dev works as expected, including in response to file modifications.

I've also modified the "template" package so that there is now a Plop script which can generate new packages from the template. I've added a CI job which generates a new package, and runs yarn build/lint/test on it, which should help to ensure that the template remains up-to-date (which is a concern @artwyman raised in another comment).

packages/pcd/webauthn-pcd/package.json

scripts/test-packaging.sh

examples/nextjs/package.json

package.json

templates/package/README.md

templates/package/templates/package.json.hbs

robknight · 2024-01-17T10:26:58Z

I've eliminated the icon generation script - it turned out to be unnecessary, tsc can just import SVG files as data URLs already.

ichub · 2024-01-17T17:50:44Z

two main things holding me back from approving this PR:

an answer to this q

confirmation that this works in render

robknight · 2024-01-17T18:09:16Z

I've eliminated the icon generation script - it turned out to be unnecessary, tsc can just import SVG files as data URLs already.

I was wrong about this :(

Looking at it again now.

…facts

robknight · 2024-01-23T20:20:14Z

two main things holding me back from approving this PR:
1. an answer to this q

The answer is that yarn dev and yarn build do something slightly different. dev builds all of the packages using a single TypeScript process using tsc --build. This is fast, and uses relatively little RAM. It makes sense to do this because, if we're running dev, we want all of the packages to be built.

yarn build just runs the build command for every workspace, whether an app or package. Each package having its own independent build command is important because sometimes we just want to build that one package. For instance, the package publishing workflow rebuilds all of the packages we're publishing individually, but we might not be publishing all of them. It's still compiling using tsc, but takes the (slower) path of running it once per package. The relative slowness is less of a problem now because it is almost never necessary to run yarn build locally. With the new tsconfig, TypeScript can pick up type definitions without performing a build.

2. confirmation that this works in render

I've deployed it to the ivan-staging environment, which seems to work.

ichub

lgtm after comments

ichub · 2024-01-23T23:01:05Z

apps/consumer-server/package.json

@@ -4,12 +4,12 @@
  "license": "GPL-3.0-or-later",
  "private": true,
  "scripts": {
-    "dev": "NODE_OPTIONS=--max_old_space_size=4096 NODE_ENV=development ts-node-dev src/main.ts",
+    "dev": "NODE_OPTIONS=--max_old_space_size=4096 NODE_ENV=development ts-node-dev --respawn --watch=.env,.env.local src/main.ts",


Here and elsewhere, I'm not sure you need to watch .env.local (as I don't think that file is used by anything), but watching .env seems like a good idea to me. I see that it is probably used in some of our projects, but not all. The reason I think it's important to get this precisely right is that otherwise it may be misleading to developers trying to understand how this project works.

ichub · 2024-01-23T23:02:09Z

apps/passport-server/package.json

-    "start:dev": "NODE_OPTIONS=--max_old_space_size=4096 NODE_ENV=development node -r source-map-support/register build/src/main.js",
-    "dev": "yarn build && nodemon -w src -e ts --exec \"yarn build && yarn start:dev\"",
+    "start:dev": "NODE_OPTIONS=--max_old_space_size=4096 NODE_ENV=development ts-node-dev --respawn --watch=.env,.env.local src/main.ts",
+    "dev:worker": "nodemon -w src/multiprocess/worker.ts --exec \"yarn tsc -p ./tsconfig.json\"",


just curious but why'd you add this?

The previous version used nodemon to watch the src directory, but was not capable of picking up changes in the packages, forcing a manual restart. ts-node-dev watches for changes in any imported module, so will restart passport-server whenever any imported package changes.

ts-node-dev only transpiles TypeScript code that gets imported, so it would not transpile the multiprocess worker, because this is only indirectly imported. So now we concurrently monitor this file for changes with nodemon, recompiling passport-server when it changes. This recompile triggers ts-node-dev to restart. So now passport-server restarts when any of its own source changes, when any package changes, or when the multiprocess worker changes.

ichub · 2024-01-23T23:02:26Z

apps/passport-server/package.json

@@ -5,13 +5,14 @@
  "private": true,
  "scripts": {
    "build": "rm -rf ./build && yarn tsc -p ./tsconfig.json",
-    "start:dev": "NODE_OPTIONS=--max_old_space_size=4096 NODE_ENV=development node -r source-map-support/register build/src/main.js",
-    "dev": "yarn build && nodemon -w src -e ts --exec \"yarn build && yarn start:dev\"",
+    "start:dev": "NODE_OPTIONS=--max_old_space_size=4096 NODE_ENV=development ts-node-dev --respawn --watch=.env,.env.local src/main.ts",


same comment as above about .env.local here

ichub · 2024-01-23T23:02:59Z

examples/zupass-feed-server/package.json

@@ -8,7 +8,7 @@
  },
  "private": true,
  "devDependencies": {
-    "@pcd/eslint-config-custom": "^0.8.0",
+    "@pcd/eslint-config-custom": ">=0.8.0",


should this be a ^ rather than a >=

ichub · 2024-01-23T23:06:28Z

packages/lib/passport-ui/convert-icons.mjs

+  if (newestIconTimestamp <= fs.statSync("src/icons/index.ts").mtimeMs) {
+    console.log("No changed icons detected");
+    return;
+  }


Noting that based on my understanding of how this stuff works I can see like some chance this could be buggy. e.g. i've sometimes encountered files that i extract from a zip file or download from the internet that have some sort of old 'creation time' that is not the same time as the time that the file was put into my file system. leaving it to you to decide whether to do something about it / create an issue / implement another solution / whatever else. Not sure how high the cost is to just run this script every build without checking if the file is outdated.

I've replaced this with a check to compare the output with the file on-disk, so it will always read the full set of icons and generate the output before comparing it.

ichub · 2024-01-23T23:06:51Z

packages/lib/passport-ui/convert-icons.mjs

some more logging in this file could be nice

ichub · 2024-01-23T23:13:22Z

scripts/new-version.sh

if this script is now in this directory, i think it's probably worth noting in the script that it's intended to be run from the root of the project (i.e. ./scripts/new-version.sh) as opposed to having the scripts directory be the working directory from which this script is run

ichub · 2024-01-23T23:14:51Z

scripts/testTemplatePackage.ts

+}
+
+main().catch((error) => {
+  process.exit(1);


maybe worth logging the error

ichub · 2024-01-23T23:21:26Z

templates/package/README.md

+
+To generate a new package, run:
+```
+yarn yarn plop --no-progress --plopfile 'templates/package/plopfile.mjs'


Suggested change

yarn yarn plop --no-progress --plopfile 'templates/package/plopfile.mjs'

yarn plop --no-progress --plopfile 'templates/package/plopfile.mjs'

?

robknight force-pushed the rob/typescript-5 branch 3 times, most recently from 8816fe0 to e585187 Compare January 10, 2024 12:04

robknight requested review from ichub and artwyman January 10, 2024 15:21

robknight changed the title ~~[WIP] Use tsc for builds~~ Use tsc for builds Jan 10, 2024

ichub reviewed Jan 10, 2024

View reviewed changes

robknight force-pushed the rob/typescript-5 branch from 44868e5 to 8800923 Compare January 11, 2024 22:09

ichub reviewed Jan 12, 2024

View reviewed changes

robknight force-pushed the rob/typescript-5 branch from de21ef6 to 50bd082 Compare January 15, 2024 09:16