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

build: clean up TS build pipeline #2828

Merged
merged 25 commits into from
May 23, 2024
Merged

Conversation

ssalbdivad
Copy link
Collaborator

@ssalbdivad ssalbdivad commented May 15, 2024

This PR addresses a big underlying problem with downstream type-perf due to .ts files being referenced from package.json.

To allow types to resolve to .ts files while in dev without affecting build output, I've set up a repo-wide set of tsconfig paths aligned to each package name, combined with any subpath exports defined in its package.json.

Changes include:

  • Adding base tsconfig.json at the repo root defining paths and common settings
  • Extend the base tsconfig.json in each package and dedupe other settings
  • Remove unnecessary nested .gitignore files
  • Replace .npmignore files with files in package.json

I'd appreciate some sanity checks on local dev working as expected etc. before this is merged.

This PR is already quite large in scope, and the type improvements it offers are quite significant, so rather than letting it continue to grow, I have a few suggestions for follow up work:

  • Ensure tsx is used throughout development and Vitest adds the vite-tsconfig-paths plugin so that imports also resolve to .ts during dev.

  • Implement a build script that creates a temporary tsconfig with compilerOptions/paths set to {} for building so that the build will fail if the tsconfig paths defined for a given package aren't also resolvable in build output. This could then replace tsup and all the corresponding configs could be removed, which would be great for deduplication and simplicity.

  • Take an additional pass at the project-specific tsconfig settings, removing as many overrides as possible to standardize behavior across the repo. In some cases these may be needed, but largely the overrides may just be an artifact of the way the tsconfigs were initially defined.

  • Migrate to module NodeNext, i.e. .js suffixes on all local imports. Kind of an eye sore, but the settings has lots of other advantages. I think you mentioned wanting to get rid of barrel files- this would be a good change accompanying that. That is unless you want to use Bundler with .ts imports or similar and then have some tool handle converting the build output for you, but if you want to use tsc which I think is ideal for a repo like this, .js endings are probably your best bet.

  • In packages/cli/src/commands/set-version.ts on line 85, there was an npmResult that was being implicitly resolved as any due to a resolution issue. That should have an explicit type, but in absence of one existing, I've explicitly typed it as any for now and added a TODO referencing this PR.

  • A few packages like CLI which probably shouldn't rely on DOM types do indirectly through utils, e.g.:

    packages/cli test:ci: ../common/src/utils/waitForIdle.ts(3,16): error TS2304: Cannot find name 'requestIdleCallback'.
    

    I've added DOM to their tsconfig.json for now to unblock the build, but eventually utils should probably either be split into a separate package for DOM APIs or have a separate entry point.

  • I had to make a few suboptimal changes to get the .abi.json files to resolve to their corresponding .d.ts with a default moduleResolution setting of "bundler" (by default with this setting and resolveJsonModule, the imports seem to resolve to the non-narrowed .json files rather than the .d.ts with narrowed types:

    packages/world/tsconfig.json: "resolveJsonModule": false,
    packages/cli/tsconfig.json: "moduleResolution": "Node",

Happy to schedule a call to go over all this in more detail- I know it's a lot!

Copy link

changeset-bot bot commented May 15, 2024

🦋 Changeset detected

Latest commit: d4eea91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 23 packages
Name Type
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/cli Patch
@latticexyz/common Patch
@latticexyz/config Patch
@latticexyz/dev-tools Patch
@latticexyz/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/protocol-parser Patch
@latticexyz/query Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
@latticexyz/store-indexer Patch
@latticexyz/store-sync Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world-modules Patch
@latticexyz/world Patch
create-mud Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
mock-game-contracts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

},
"files": [
"dist"
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just noting my own learnings here:
we don't need to include everything from .npmignore here because by default, npm picks up package.json as well as README (with any extension): https://docs.npmjs.com/cli/v10/configuring-npm/package-json#files

packages/cli/package.json Outdated Show resolved Hide resolved
packages/common/package.json Outdated Show resolved Hide resolved
packages/store/package.json Outdated Show resolved Hide resolved
@holic holic changed the title fix: avoid .ts in build output, remove some redundant .gitignores build: clean up TS build pipeline May 23, 2024
@holic holic marked this pull request as ready for review May 23, 2024 16:30
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.

3 participants