Switch to nodejs export conditions from TS project references #67

theseanl · 2024-01-20T13:15:03Z

Previously, typescript project references were used to wire imports between workspace packages. During compilation, it allows one to resolve any workspace modules imported from another workspace module to their respective declaration files, and if it hasn't been built, it automatically tries to build the dependent module first. Also, in IDE, TS project references allows IDEs to resolve imports to their source .ts file instead of the .d.ts file as during the compilation; this allows one to get feedback directly from IDE when something is changed from a dependent workspace package without the need of re-building it.

However, it seemed that type information available from .ts and .d.ts were sometimes different, so that there was no error caught on IDE but the build fails. I believe it must be a bug of TS, or maybe due to a defective d.ts generated for yet another issue in the meantime, but this has been pretty confusing. Also, having to reference .d.ts to build requires a dependency graph to be acyclic, which hampers quickly implementing and testing features (regardless of the end output dependency graph being acyclic or not).

This PR tries to get rid of such issues by ditching TS project references and utilizing nodejs native package.json export conditions. Note that export conditions were already being used to support consuming the package in both esm and cjs format. We add an artificial condition "_bundle", and use the respective bundler's export conditions feature https://esbuild.github.io/api/#conditions and https://www.typescriptlang.org/tsconfig#customConditions to achieve a similar effect as what was provided by TS project references: IDE resolve any workspace package imports to their respective Typescript source file, not their build output .d.ts files. Moreover, now the compilation also consults the original .ts source files instead of .d.ts, so the build order does not need to be topologically sorted.

We lose an ability to compile only the necessary packages by abandoning TS project references, but I believe the number of packages are not too large, so we can manually accommodate it or just run a full build. If this feature is strictly needed, one may adopt a dedicated task runner such as Make.

+) This PR also tweaks build commands so that it works equally on Windows.

Previously, typescript project references were used to wire imports between workspace packages. Now this is done by package.json export conditions. This nodejs feature has already been used to support consuming packages in both esm and cjs format. We add an artificial condition "_bundle", and use the respective bundler's export conditions feature https://esbuild.github.io/api/#conditions and https://www.typescriptlang.org/tsconfig#customConditions to achieve that IDE and TS compiler resolve any workspace package imports to their respective Typescript source file, not their build output .d.ts files. This has an additional benefit of reducing bundle size of cjs build. Previously, CJS build operated on ESM build output; They fed dist/index.js files to esbuild. This changes this so that esbuild directly operates on Typescript source files even in CJS build. As esbuild will have more information, it will be able to better tree-shake unused codes.

siefkenj · 2024-01-20T15:00:45Z

Could you reformat the JSON with Prettier, please? It should be 4 space indentation.

also used double quotes for interoparabilities with Windows

theseanl · 2024-01-22T11:04:54Z

I just learned that Typescript project references are not transitive. Relevant discussions are at microsoft/TypeScript#30608, microsoft/TypeScript#46153. This could be a source of weird baheviors I have been experiencing with tsc -b.

siefkenj · 2024-01-22T16:29:18Z

packages/structured-clone/build.js

@@ -19,6 +19,7 @@ import { isCjsPackage } from "../../scripts/esbuild-module-check.mjs";
        format: "esm",
        target: "node14",
        external: [...explicitDeps],
+        conditions: ["_bundle"],


I don't like the idea of adding a custom condition. Why is this needed?

It is a key addition of the PR that makes the build work without using Typescript project references.

TS project reference allows Typescript to resolve import statements that import from another workspace package, such as @unified-latex/unified-latex-types, to their respective .ts source files or .d.ts declaration files depending on circumstances (more on it in the original post).

If TS project reference is removed, Typescript and ESBuild will follow the native nodejs module resolution algorithm, and it involves finding matching exports condition, which will be import if without _bundle, and it will then look for /dist/index.js file, which may or may not exist at that point. _bundle is there to point it to the .ts source files.

Frankly I don't like this as well, it feels like this custom condition is not designed for a use-case like this. I'll try to consult how other large Typescript monorepos are doing. That said, it was definitely an improvement in terms of dev experience because there's no longer any surprising build errors.

siefkenj · 2024-01-22T16:29:48Z

packages/unified-latex-builder/package.json

+        "clean": "rimraf ./dist",
+        "compile:tsc": "tsc",
+        "compile:esm_and_cjs": "node build.js",
+        "compile": "run-p compile:tsc compile:esm_and_cjs",


This is great. I didn't know about this utility!

I don't like npm scripts at all, I don't like npm or Typescript project references trying to be a task runner!
It'd be nice to maximally parallelize the entire build, but currently there's no way to run npm scripts on each workspace in a parallel way.

My other PR uses wireit to do this, but it turns out the build didn't get sped up at all...(it was slowed down a little bit). You can look at it and see if I did something obviously wrong...

siefkenj · 2024-01-22T16:36:04Z

packages/unified-latex/package.json

@@ -18,19 +18,23 @@
    ],
    "exports": {
        ".": {
+            "_bundle": "./index.ts",


What would happen if we just pointed to the ts files via "import"? This file gets rewritten when packaging, so that might be a solution without introducing a new condition.

import field is what will be used when a consumer of this package installs it from npmjs and import it via import '@unified-latex/unified-latex. If it pointed to .ts file, because .ts files aren't present in the production package, consumers won't be able to import anything.

This won't matter, because the package.json file is completely rewritten for production. You can look in the scripts/make-package.mjs file for what happens when preparing to publish the package on npm.

Oh, didn't know that. Just tried "import": "./*.ts", and it seems that all the builds succeeds, but there's one nuisance, tests in unified-latex-cli invokes generated .mjs files directly and it needs the correct import values to locate imported modules. Not sure how this can be fixed.

Hmmm...I suppose we could bundle all the deps when the cli is built. It seems reasonable that unified-latex-cli be a standalone thing.

We can make import point to .ts, and real_import or whatever name to point whatever the previous import did, and call node with node -C real_import in cli testing. Then we will still be able to remove custom conditions from tsconfig and esbuild build scripts

Would that require real_import to be added to every package.json? Either way, it seems reasonable. Let's call it dist_import or prebuilt

Presumably yes, it should be added to every package.json that unified-latex-cli depends on

Since package.json is re-written before publishing, we are free to point `import` condition to `.ts` source files. However, CLI tests need to be able to resolve to `dist/*.js` files, so a custom condition `prebuilt` pointing to those are kept in every package.json.

theseanl added 2 commits January 20, 2024 21:46

Merge branch 'main' into ditch-ts-project-references

da0e74c

theseanl force-pushed the ditch-ts-project-references branch from 17dd95f to da0e74c Compare January 20, 2024 13:56

theseanl added 3 commits January 21, 2024 00:48

build: use _bundle export condition on esm bundle as well

b69517b

chore: make prettier format json

9035e2a

also used double quotes for interoparabilities with Windows

build: set forceConsistentCasingInFileNames

768a308

siefkenj reviewed Jan 22, 2024

View reviewed changes

theseanl force-pushed the ditch-ts-project-references branch from 7fa26c2 to af5e25d Compare January 23, 2024 23:34

siefkenj merged commit 4811a0b into siefkenj:main Jan 24, 2024

siefkenj mentioned this pull request Jan 24, 2024

Default arguments support #24 #62

Open

theseanl mentioned this pull request Jan 28, 2024

Revert to custom export conditions #68

Closed

Switch to nodejs export conditions from TS project references #67

Switch to nodejs export conditions from TS project references #67

Uh oh!

Conversation

theseanl commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siefkenj commented Jan 20, 2024

Uh oh!

theseanl commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theseanl commented Jan 20, 2024 •

edited

Loading

theseanl commented Jan 22, 2024 •

edited

Loading