Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor build and typescript stuff in v2 #32

Merged
merged 34 commits into from
Oct 27, 2022
Merged

Refactor build and typescript stuff in v2 #32

merged 34 commits into from
Oct 27, 2022

Conversation

josephjclark
Copy link
Collaborator

@josephjclark josephjclark commented Oct 7, 2022

Want to do this off the main v2 branch.

This MR makes a number of build and typing changes. The headlines are:

  • Use tsup for building
  • Simplify tsconfig usage
  • Add explicit type checking to src and test (although not on compiler because there's a few issues there...)
  • Run prettier

Closes #30 #29

I think this sets some good precedence for the monrepo work.

I am in the process of applying this to each package for a nice, light, consistent setup.

Details and discussion points follow.

tsup

tsup is great. It adds a bunch of config and .d.ts support to esbuild. It greatly simplifies the build setup (also we can get in deep if we have to ie in describe-package).

The output bundle is super clean. It's just one file and handles dependencies properly. It looks like ESM and has minimal wrangling (just a bit at the top level).

tsup includes a .d.ts file which looks to have everything I want in it. This does slow down the build because esbuild has to wait for the ts compiler to run (with dts disabled, esbuild just strips out the ts markup and does no validation). I am not seeing a problem, we could consider excluding the d.ts in watch or NODE_ENV=dev modes.

There's very little config. Each package calls tsup in the build command, passing in a common config file. The common config is delightfully small.

I don't believe we need sourcemaps for these packages. The JS itself is readable, which will facilitate debugging.

TSconfig

tsup handles all the input and output processing of the build, so really our tsconfig is only used to capture ts strictness and validation rules.

Type Checking

The build will fail if any ts errors are detected. This is a good thing (at least in CI).

I've added an explicit type check script to each project. It doesn't cost very much and feels like a very useful option to have. It just calls tsc --noEmit.

Ideally we would run the type check before building so that we can quickly and clearly identify type issues. But the typecheck actually relies on the build for workspace sibling dependencies to resolve. So that's a bit pointless really.

Workflow Diagram

Has not been touched

Describe Package

I've removed the rollup build and added a simple esbuild, which builds for node modules. I haven't touched the existing esbuild, save to add a comment and map it to a build:worker script.

AIUI the worker isn't working anyway, so while I don't want to remove the existing worker build, I also don't want to open the can of worms of tampering around with it, either!

I think we should be able to simplify the esbuild for this repo but I'll defer that problem until later.

@josephjclark
Copy link
Collaborator Author

Some packages declare pnpm as a required in engines. But I don't think pnpm is needed in the built packages - so shouldn't we remove this?

@josephjclark
Copy link
Collaborator Author

josephjclark commented Oct 7, 2022

What do to about file extensions?

Technically a node16+ ESM module should include .js extensions in relative imports, ie import ./index.js

We've been able to get away with this (I think) by making typescript think we're building CJS module, which doesn't require file extensions. Then the actual build emits ESM files.

But this isn't right really, it means typescript can't do proper type checking.

Which means, I think, unfortunately and annoyingly, that we have to add .js to all relative imports.

EDIT: Actually ,we work around this in in node with --experimental-specifier-resolution passed to the command line. Surely there's some way to make TS respect this...

EDIT 2: This magical combination seems to work:

"module": "es2020",
"moduleResolution": "node"

@josephjclark
Copy link
Collaborator Author

I really want to work out typings on this branch:

  • In my package I declare types.d.ts with a bunch of types in it.
  • I point types in tsconfig to that file
  • Those types are available globally to the package without explicit import
  • The exported d.ts includes those types

That last bit in particular is being problematic.

@josephjclark josephjclark changed the title V2 tidy Refactor build and typescript stuff in v2 Oct 7, 2022
@josephjclark
Copy link
Collaborator Author

Really happy with this now: the build is very minimal yet scales well across projects. Very happy with tsup and esbuild. I've run various small tidyups (including sorting prettier out, finally) and I'm happy with how things look.

@josephjclark josephjclark marked this pull request as ready for review October 7, 2022 15:26
@josephjclark
Copy link
Collaborator Author

Just noticed that compiler still has a rollup config file - pretty sure this can do (I just need to double check)

@stuartc
Copy link
Member

stuartc commented Oct 11, 2022

@josephjclark this is wonderful 👍

@josephjclark josephjclark changed the base branch from logger to v2 October 27, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants