Much faster dev builds, lower memory usage, working TypeScript navigation in VSCode #1398

robknight · 2024-01-08T09:13:32Z

I've given a reasonably full history of the changes made below. The gist is that development builds are now much faster with vastly lower RAM usage, VSCode seems to have no trouble keeping track of types, and I've eliminated some annoyances in the way that TypeScript declaration files differed in their locations in the dist directory between packages.

This PR makes a set of changes intended to solve some of the recent problems with:

a) Development build times
b) Development build memory usage
c) VSCode/TypeScript language server losing track of TypeScript types (#1387)
d) VSCode cmd-click going to type declarations rather than the original source definition

These problems began following the packaging changes designed to make packages easier for third-party developers to use. However, they worsened developer experience when working in the monorepo.

Prior to the packaging change, each package included full TypeScript source, and each package.json file simply referenced the TypeScript source in the types field. This is how the TypeScript language server (and VSCode) were able to track types between packages easily, allowing cmd-click navigation and rapid reconciliation of type changes that affect multiple packages.

Post-change, the types field pointed to a type declaration file (e.g. dist/types/index.d.ts). This is great for third-party package consumers, because they no longer needed to be able to parse TypeScript files or worry about TypeScript dependencies. However, it was bad for monorepo development because:

a) Type declaration files are build artifacts, so they don't exist unless you have run yarn build
b) They also don't get updated unless you run yarn build, so type changes don't propagate between packages quickly
c) Cmd-clicking on a type in VSCode takes you to the type declaration file, not the original type in the relevant source file

(c) was fixed by using TypeScript declaration maps, which also necessitated switching to use tsc to generate types instead of tsup. This meant that each build command involved running tsup to transpile TS->JS, then running tsc to generate the type declarations and declaration maps.

In a separate change, we also increased the number of packages substantially, by splitting out the React components from the PCD packages.

This got us to a point where, in order to do a development build, we had to run tsup on each package to generate JS, then run tsc on each of those packages. This was very slow. Parallelizing the processes helped with speed, but causes very high RAM usage - dangerously high on 8GB RAM, and still bad enough on 16GB RAM that it would cause a noticeable slowdown for around a minute.

For development, we were running tsup in watch mode, meaning one watcher process per package. With the increased number of packages, the watcher processes started using a lot of RAM (and these watcher processes would also spawn tsc as a child process whenever they needed to rebuild types, which they did frequently).

The last change I made replaced tsup's watch functionality with a separate script that watched the entire monorepo for changes, starting a new build job every time a change was detected. Combined with Turborepo caching, this meant that development builds were reasonably fast again, and mostly did not require spawning many new processes. This eliminated most of the pathological memory usage cases.

However, there was still a problem in that builds would delete and re-create TypeScript declaration files, which would cause the TypeScript language server to lose track of them. VSCode would frequently complain that it could not find type declarations even when the new files clearly existed, and only restarting the language server would fix it.

At this point, I went back to the drawing board, to see how other projects solve this problem. The answers were quite varied, and the "how do we make nice packages for third-party developers while also having optimal monorepo developer experience" problem is also a problem for others.

The main thing that I learned was that the TypeScript language server runs a kind of "shadow" compilation of your project in RAM. If you have a TypeScript configuration that works well with this, the TypeScript language server can map out your entire project, with all of the cross-package type dependencies, even if the type declaration files don't exist on disk. This requires a few things to be just-so in the tsconfig.json file:

rootDir and outDir must be set in compilerOptions
The package.json types field must point to the location inside the outDir where the type declaration files will be generated (which requires setting declarationDir)
TypeScript "references" must be used to tell TypeScript which other packages in the monorepo are dependencies for the current package
A separate tsconfig.build.json file is used to exclude test file types from the packaged builds

TypeScript references are entries in a tsconfig.json that point to other packages. These exactly mirror the dependencies in package.json, and there is an issue for TypeScript to infer them automatically. In the meantime, it's easy to generate them programatically.

With all of these things done, we get:

TypeScript/VSCode can track types across the whole project, even when the declaration files don't exist, getting us back to the developer experience we had when we were bundling the TypeScript source
Because we don't need to build type declaration files for development, development mode no longer does this - instead, we only have to transpile TS->JS, which is much faster
Consistent configuration between packages means that the types always go in the same place; there used to be a weird issue where types would have to be placed in dist/types/src rather than dist/types for some packages, because the rootDir was . by default rather than ./src

I've done a little bit of benchmarking, and this has improved development build times quite a bit:

	Old builds (pre packaging fix)	Status quo	This PR
Cold start	77s	79s	27s
Warm start	29s	32s	16s

This measures the time it takes from starting yarn dev to having a running passport-server. "Cold start" is a fresh checkout, "warm start" is running yarn dev to completion, stopping it, then running it again with no changes.

I haven't benchmarked memory usage with any precision, but it is obviously lower. This is because there is only a single watcher process now, and we never have to spawn tsc to generate type declaration files during development builds.

A full build now has two distinct phases: transpilation (build:ts) and type declaration generation (build:types). build:ts is what we run during development. When we want to do a full build, we run both built:ts and build:types - the top-level yarn build command just runs both.

passport-server running in dev mode now uses ts-node-dev with the --transpileOnly flag. This means that it does not do any type generation/checking, and therefore doesn't need declaration files to exist on-disk. This also speeds up startup substantially. I think this is an improvement given that full builds still do type checking, and VSCode will accurately report any type errors.

The TypeScript references mentioned earlier do need to be kept up-to-date. This is fairly easily done by running yarn fix-references - this just ensures that the tsconfig.json references match the dependencies in package.json. yarn check-references has been added to CI to make sure we don't forget to do this.

artwyman · 2024-01-09T06:17:09Z

packages/pcd/rln-pcd/src/RLNPCD.ts

@@ -20,7 +20,7 @@ import { Identity } from "@semaphore-protocol/identity";
 import JSONBig from "json-bigint";
 import { Proof, RLN, RLNFullProof } from "rlnjs";
 import { v4 as uuid } from "uuid";
-import verificationKeyJSON from "../artifacts/16.json";
+import verificationKeyJSON from "./16.json";


Interesting. Why did this file need to move when the artifacts of other ZK PCDs didn't?

For TypeScript's language server to be able to generate its own in-memory representation of the full type structure, we need tsconfig.json to accurately reflect the structure of the code base. This means that rootDir needs to be the actual source root, and files cannot be included from outside of it. The JSON file is the only artifact file that gets included by TypeScript, and so has to move to somewhere inside of the source root, or TypeScript will fail to generate types.

Looking deeper, it seems the answer is that the other ZK PCDs did in fact need this change, and I found the point where the file is moved and the artifact generation script is updated. I missed that last night, possibly because I was looking at the -ui package instead of the main PCD package. Sorry for the confusion, I understand the reason for this change.

artwyman · 2024-01-09T06:22:52Z

I only skimmed the changes since they seem to be mostly global/automated, but this all sounds pretty good.

I tested this branch on my low-memory machine and at least a warm build had similar levels of memory usage as viewed in Activity Monitor. yarn dev seemed faster. yarn build seemed slower due to the new build:types phase which doesn't seem to benefit from Turborepo. I also noticed that it was mostly running a command which was deleting and recreating type files. I wonder why there can't be last-modified-time-based dependency checking to avoid doing that for every package when nothing has changed. Or if Turborepo caching can be applied for a similar effect.

In the interim, would manually running build:ts have the effect skipping this step? Would it also bypass type checking?

I'm a bit nervous about skipping type checking in dev mode, from the general philosophy of always wanting to catch errors as early as possible, and not waste time debugging an issue which a type error would've caught. VSCode will show me errors in code I'm looking at actively, but not more spread-out results. Though maybe I should be paying more explicit attention to the "Problems" window in VSCode to address that? If we think the code/tools are now clean enough that it should normally be empty.

robknight · 2024-01-09T08:18:59Z

I tested this branch on my low-memory machine and at least a warm build had similar levels of memory usage as viewed in Activity Monitor. yarn dev seemed faster. yarn build seemed slower due to the new build:types phase which doesn't seem to benefit from Turborepo.

This should not be the case. If I run yarn build twice, the second time will cache both the transpilation and type generation phases.

I also noticed that it was mostly running a command which was deleting and recreating type files. I wonder why there can't be last-modified-time-based dependency checking to avoid doing that for every package when nothing has changed. Or if Turborepo caching can be applied for a similar effect.

Turborepo caching ought to handle this, as per the above. I'm not sure why it would not be doing that here.

In the interim, would manually running build:ts have the effect skipping this step? Would it also bypass type checking?

Yes, this just does transpilation.

I'm a bit nervous about skipping type checking in dev mode, from the general philosophy of always wanting to catch errors as early as possible, and not waste time debugging an issue which a type error would've caught. VSCode will show me errors in code I'm looking at actively, but not more spread-out results. Though maybe I should be paying more explicit attention to the "Problems" window in VSCode to address that? If we think the code/tools are now clean enough that it should normally be empty.

The truth is that our dev mode from two months ago didn't do type checking either - we just had a tsup --watch command running for every package. We only started type "checking" in development because the lack of type declaration files was breaking the developer experience. With TypeScript references, we don't need declaration files on-disk during development, and so we don't need to run tsc every time a package changes. This means that we're not type-checking on every recompile in development, but this is just what we were doing before my packaging change, and in practice this wasn't causing us a problem back then.

Using ts-node-dev with --transpile-only to run the apps does skip a step where the apps would import type declaration files from the packages. There was a time when those type declarations came directly from the TypeScript sources, and so perhaps this would have caught type errors as they were introduced. I don't personally recall getting type errors this way, though - I only ever notice them in VSCode, or on rare occasions when running a build.

There are other approaches we could try, that might produce better trade-offs. For instance, we could upgrade to TypeScript 5, create a top-level tsconfig.json containing references to all of the packages, and use that to generate the types for the entire project in one go (this approach is not supported in TypeScript 4.x, which is what we are using). We could then run this in "watch mode" to regenerate types on-the-fly. If what we want is a process running in the console that tells us about type errors immediately, I think this would do it.

artwyman · 2024-01-09T19:54:24Z

I did some more resting and it seems Turborepo is actually caching the type generation. I may not have seen that yesterday due to confusion when switching between branches, sorry. Experimentally, it seems it also behaves the way I'd expect if I change a single file. All the type-generation commands scroll by, but only one of them pauses for any meaningful time, suggesting only one was unached.

I think I may never have experienced the "original" dev mode. I saw some evidence that the auto-rebuild wasn't reliable (may've depended on whether the modified source was inside passport-client or not), so defaulted to always running yarn build manually in the package I was working on, after yarn test. This speed vs. safety tradeoff is possibly a philosophical difference. I've spent most of my career in statically typed languages, and see a lot of value in using the type system to catch potential errors. I consider code which doesn't type-check to be invalid code and don't think it should ever be running, so I'm annoyed if I ever find myself debugging an issue which turns out to be from invalid code. Other people's expectations may vary. It sounds like I can continue to follow a safer workflow manually if I want to regardless, so I don't need to block on this, but I'll argue passionately about the philosophy of finding bugs as early in the toolchain as possible if it's a conversation you ever want to have. :)

Separately: while experimenting I also saw this error pop up in VSCode, which I thought you might want to look at:

File '/Users/artwyman/dev/zupass/packages/pcd/zk-eddsa-event-ticket-pcd/test/ZKEdDSAEventTicketPCD.spec.ts' is not under 'rootDir' '/Users/artwyman/dev/zupass/packages/pcd/zk-eddsa-event-ticket-pcd/src'. 'rootDir' is expected to contain all source files.
  The file is in the program because:
    Matched by include pattern 'test' in '/Users/artwyman/dev/zupass/packages/pcd/zk-eddsa-event-ticket-pcd/tsconfig.json'

ichub

Excellent documentation in the PR description! Thanks for digging deeper into this.

I left some comments with questions and small suggestions. LMK what you think. It looks like you're still in the middle of some changes so I've held off on testing this locally. LMK when you would want me to test locally.

re @artwyman's comment

Other people's expectations may vary. It sounds like I can continue to follow a safer workflow manually if I want to regardless, so I don't need to block on this, but I'll argue passionately about the philosophy of finding bugs as early in the toolchain as possible if it's a conversation you ever want to have. :)

I agree w/ this - it would be really nice to catch type errors as early as possible. I also wouldn't block on this here, but it's worth creating an issue for imo.

Re @robknight's comment:

There are other approaches we could try, that might produce better trade-offs. For instance, we could upgrade to TypeScript 5, create a top-level tsconfig.json containing references to all of the packages, and use that to generate the types for the entire project in one go (this approach is not supported in TypeScript 4.x, which is what we are using). We could then run this in "watch mode" to regenerate types on-the-fly. If what we want is a process running in the console that tells us about type errors immediately, I think this would do it.

How much work would it take to migrate do you think?

ichub · 2024-01-09T20:52:26Z

.vscode/settings.json

@@ -29,6 +29,7 @@
    }
  },
  "eslint.rules.customizations": [
-    { "rule": "prettier/prettier", "severity": "off" }
+    { "rule": "prettier/prettier", "severity": "off" },


ichub · 2024-01-09T20:52:34Z

.vscode/settings.json

@@ -29,6 +29,7 @@
    }
  },
  "eslint.rules.customizations": [
-    { "rule": "prettier/prettier", "severity": "off" }
+    { "rule": "prettier/prettier", "severity": "off" },
+    { "rule": "import/no-unresolved", "severity": "off" }


why do you need this line?

This is because TypeScript declaration files are a strange thing, part source (when they're being imported by other packages) and part build artifact (when they're sitting quietly in the dist directory).

The TypeScript language server is smart enough to build its own model of the types, and this is how VSCode is able to do code navigation. But eslint isn't smart enough to do this, and will flag missing type declaration files as a lint error because it can't resolve the TypeScript dependency unless it can see the type declaration files. This means that the codebase fails linting on a fresh checkout, which seems wrong (and is unfixable unless we check the type declarations in, which also seems wrong as they are build artifacts).

This configuration change only affects whether VSCode will display the error, not whether eslint will detect it, so it still gets picked up when running yarn lint. In CI, yarn lint runs after yarn build, which means that the type declaration files will be present.

Finally, if you're actually trying to import a package that doesn't exist, or which doesn't have an index.ts, or is not correctly configured to export types, then you will get a different (and better) error from the TypeScript language server in VSCode anyway, making this one redundant.

makes sense!

ichub · 2024-01-09T20:54:03Z

package.json

+    "clean:workspaces": "turbo run clean --parallel && yarn",
+    "knip": "knip --no-gitignore",
+    "check-references": "workspaces-to-typescript-project-references --check",
+    "fix-references": "workspaces-to-typescript-project-references && prettier -w \"{apps/*,packages/*/*}/tsconfig.json\""


why does this script run prettier?

Because it causes changes to a JSON file but doesn't format the JSON file in the same way that prettier would, so we prettify it after the change is made.

ichub · 2024-01-09T20:54:22Z

package.json

@@ -40,6 +43,7 @@
  },
  "dependencies": {
    "@changesets/cli": "^2.26.0",
+    "@monorepo-utils/workspaces-to-typescript-project-references": "^2.10.4",


cool project. i wonder what else is in @monorepo-utils that we could yoink

ichub · 2024-01-09T20:57:53Z

packages/lib/util/tsconfig.json

comment unrelated to this particular file but it's where I had the thought so it's where I'm writing it: given the complexity of setting up a package in this monorepo, i wonder if it would make sense to add an example 'template' package that is intended to be:

kept up to date with the latest modifications to how packages ought to work in our monorepo

able to be copy-pasted almost verbatim when setting up a new package in the monorepo

Adding documentation there about all the choices that may seem arbitrary and confusing to a new developer (as opposed to in every single package) may be a good way to keep track of what's going on. Your PR description here was really good, and could be split up into such documentation.

Furthermore, it may make reviewing / proposing future changes to the packaging setup simpler.

I've included this in the new PR.

packages/tools/tsconfig/ts-library.json

packages/ui/webauthn-pcd-ui/src/CardBody.tsx

scripts/watch.ts

.github/workflows/build-and-test.yml

robknight · 2024-01-09T21:20:01Z

I did some more resting and it seems Turborepo is actually caching the type generation. I may not have seen that yesterday due to confusion when switching between branches, sorry. Experimentally, it seems it also behaves the way I'd expect if I change a single file. All the type-generation commands scroll by, but only one of them pauses for any meaningful time, suggesting only one was unached.

Good, that matches my experience.

I think I may never have experienced the "original" dev mode. I saw some evidence that the auto-rebuild wasn't reliable (may've depended on whether the modified source was inside passport-client or not), so defaulted to always running yarn build manually in the package I was working on, after yarn test. This speed vs. safety tradeoff is possibly a philosophical difference. I've spent most of my career in statically typed languages, and see a lot of value in using the type system to catch potential errors. I consider code which doesn't type-check to be invalid code and don't think it should ever be running, so I'm annoyed if I ever find myself debugging an issue which turns out to be from invalid code. Other people's expectations may vary. It sounds like I can continue to follow a safer workflow manually if I want to regardless, so I don't need to block on this, but I'll argue passionately about the philosophy of finding bugs as early in the toolchain as possible if it's a conversation you ever want to have. :)

I would agree that finding bugs as early as possible is good. Two things work against us for development builds:

Type generation is pretty slow
We really need rebuilds to be fast, since we want to refresh the page rapidly

(2) doesn't apply so much for running a full build, or for running tests, linting, or anything else. We also have VSCode checking for type errors, and so having the watcher process run the same checks that VSCode (or rather, the TypeScript language server) is already doing seems wasteful, especially because the watcher process can only do it quite inefficiently (by spawning a new process to compile the types, whereas the language server already has all of the details cached in RAM and can update much much faster).

I think it makes sense to treat the language server as the development mode type-checker, rather than have the transpiler also do type checking.

Separately: while experimenting I also saw this error pop up in VSCode, which I thought you might want to look at:

File '/Users/artwyman/dev/zupass/packages/pcd/zk-eddsa-event-ticket-pcd/test/ZKEdDSAEventTicketPCD.spec.ts' is not under 'rootDir' '/Users/artwyman/dev/zupass/packages/pcd/zk-eddsa-event-ticket-pcd/src'. 'rootDir' is expected to contain all source files.
  The file is in the program because:
    Matched by include pattern 'test' in '/Users/artwyman/dev/zupass/packages/pcd/zk-eddsa-event-ticket-pcd/tsconfig.json'

I've fixed this. Mostly for aesthetic reasons, I wanted types to be exported to dist/types/index.d.ts, which requires telling TypeScript that the rootDir is src. But this excludes tests from the rootDir, which TypeScript will then complain about (though this has no negative effects, since we don't want to transpile tests or generate types from them). Making . the rootDir will stop TypeScript from raising this error, at the cost of causing our types to move to dist/types/src/index.d.ts. As I said, purely an aesthetic difference, so not worth carrying an annoying error message for it.

This file causes hard-to-debug issues, and is generally unhelpful when we are trying to generate types.

… doesn't exist

ESLint complains that it can't resolve TypeScript dependencies when it can't find the .d.ts files, even for dependencies within the same monorepo. This is annoying as these don't exist out-of-the-box, meaning that ESLint fills VSCode up with red squiggly lines on a fresh checkout. This turns the rule off in VSCode. `yarn lint` will still detect the errors, and so will notice missing declarations when run from the command line or in CI.

robknight · 2024-01-09T22:36:39Z

re @artwyman's comment

Other people's expectations may vary. It sounds like I can continue to follow a safer workflow manually if I want to regardless, so I don't need to block on this, but I'll argue passionately about the philosophy of finding bugs as early in the toolchain as possible if it's a conversation you ever want to have. :)

I agree w/ this - it would be really nice to catch type errors as early as possible. I also wouldn't block on this here, but it's worth creating an issue for imo.

I agree in principle. However, the the TypeScript language server is already doing this, so long as VSCode is running. Agreed with creating an issue to come back to this later.

Re @robknight's comment:

There are other approaches we could try, that might produce better trade-offs. For instance, we could upgrade to TypeScript 5, create a top-level tsconfig.json containing references to all of the packages, and use that to generate the types for the entire project in one go (this approach is not supported in TypeScript 4.x, which is what we are using). We could then run this in "watch mode" to regenerate types on-the-fly. If what we want is a process running in the console that tells us about type errors immediately, I think this would do it.

How much work would it take to migrate do you think?

I've just tried migrating to TypeScript 5.3.3 and it seems quite easy. However, adding a whole different watcher seems like it could involve changing how we do watching in general.

I did consider this option earlier on. It could be possible to use the TypeScript watcher to do almost everything: both transpilation and type generation. The only problem is that TypeScript won't do separate ESM/CommonJS builds, and this is something we need for type packaging. Now, maybe that's OK for development mode and not for packaging, but I'd want to take some time to reassure myself about that. I thought that fixing our existing tsup based approach, using Turborepo's cache to make things go fast, might be better than using tsc and incremental builds as an alternative means of achieving the same thing. I can try it, though. I'd be tempted to bank the performance improvement from this PR, and look at that approach as an enhancement. Most of the changes in this PR are improvements to TypeScript config that would be necessary to support that approach anyway, so it would not end up being a waste.

Closes #1387 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: ```json "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: ```json "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: ```json { "extends": "./tsconfig.json", "compilerOptions": { "outDir": "dist/esm", "module": "ESNext", "tsBuildInfoFile": "./tsconfig.esm.tsbuildinfo" }, "include": ["src", "src/*.json"] } ``` ```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.

robknight requested review from artwyman and ichub January 8, 2024 09:13

artwyman mentioned this pull request Jan 9, 2024

Prevent recurrence of generic-pool issue #1400

Merged

artwyman reviewed Jan 9, 2024

View reviewed changes

ichub reviewed Jan 9, 2024

View reviewed changes

robknight added 21 commits January 9, 2024 23:03

WIP

d939203

WIP

ee1c47f

References now work; VSCode picks up types even when not generated

06d9e02

Source and types both build from clean start

67e8ef1

Prettify tsconfigs

ebd8b0a

Tidying up

75e51f5

Make WebAuthnPCD work like other packages

3d4ba76

Use build:ts and build:types for build and dev commands

28e6613

Move types to subdirectory in dist

6165815

Watch mode works for consumer/passport; not sure about other apps

cd80359

Fix tsconfig file

8f37c43

Add and check workspace references

3aec04d

Remove unnecessary tsconfig file

9110a7e

Exit build script if any step fails

02044e2

Remove tsconfig.tsbuildinfo before generating types

cd94072

This file causes hard-to-debug issues, and is generally unhelpful when we are trying to generate types.

Move artifacts back so that they can be packaged properly

e506117

Remove old script entries

0cd248d

Tidy up commented-out eslint rule

de210f4

Tidying

bdc1e77

Update README

4c79485

Use -f flag when deleting tsbuildinfo file so it doesn't fail if file…

ef48784

… doesn't exist

robknight added 9 commits January 9, 2024 23:03

Use tsconfig.build.json for type export

ff3ed2d

Remove accidential declaration file

034a9fb

Restore dev:apps

8544619

Fix artifact paths in tests

20461ad

Restart apps when environment file changes

eafe6c8

Clean before publishing

a9cc263

Types have to live in dist/types/src after all

d66f943

Added comment explaining composite mode

61e1537

robknight force-pushed the rob/tsconfig-references branch from f28c77e to 61e1537 Compare January 9, 2024 22:48

robknight mentioned this pull request Jan 10, 2024

Use tsc for builds #1403

Merged

robknight closed this Jan 10, 2024

mcmire mentioned this pull request May 28, 2024

tsup's code splitting feature makes core packages difficult to debug MetaMask/core#4333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Much faster dev builds, lower memory usage, working TypeScript navigation in VSCode #1398

Much faster dev builds, lower memory usage, working TypeScript navigation in VSCode #1398

robknight commented Jan 8, 2024 •

edited

Loading

artwyman Jan 9, 2024

robknight Jan 9, 2024

artwyman Jan 9, 2024

artwyman commented Jan 9, 2024

robknight commented Jan 9, 2024

artwyman commented Jan 9, 2024

ichub left a comment

ichub Jan 9, 2024

ichub Jan 9, 2024

robknight Jan 9, 2024 •

edited

Loading

ichub Jan 10, 2024

ichub Jan 9, 2024

robknight Jan 9, 2024

ichub Jan 9, 2024

ichub Jan 9, 2024

ichub Jan 9, 2024

ichub Jan 9, 2024

robknight Jan 10, 2024

robknight commented Jan 9, 2024

robknight commented Jan 9, 2024 •

edited

Loading

Much faster dev builds, lower memory usage, working TypeScript navigation in VSCode #1398

Much faster dev builds, lower memory usage, working TypeScript navigation in VSCode #1398

Conversation

robknight commented Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman commented Jan 9, 2024

robknight commented Jan 9, 2024

artwyman commented Jan 9, 2024

ichub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robknight Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robknight commented Jan 9, 2024

robknight commented Jan 9, 2024 • edited Loading

robknight commented Jan 8, 2024 •

edited

Loading

robknight Jan 9, 2024 •

edited

Loading

robknight commented Jan 9, 2024 •

edited

Loading