-
Notifications
You must be signed in to change notification settings - Fork 211
fix(templates): fix templates tsc errors and add test #2036
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
Conversation
`pnpm add --workspace-root --save-dev @types/debug@4.1.7 @types/prettier@2.7.2`
`pnpm --filter client add --save-dev @types/react-dom@18.2.7`
This test specifies the main tag, but there's a problem with that. When we make a breaking change in our library and fix the templates along with it, this test fails. Instead of main, we should use locally packed dependencies.
|
@@ -24,6 +24,7 @@ | |||
"viem": "1.14.0" | |||
}, | |||
"devDependencies": { | |||
"@types/react-dom": "18.2.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume any TS errors here come from @latticexyz/dev-tools
, but it doesn't make sense to me why we'd need to add the dependency here if the dev tools is compiled. Maybe there's something off with dev tools exports of types? Or with tsconfig for either dev tools or vanilla template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review!
dev-tools is compiled into JS files, but I confirmed that TS files are referred to by template projects based on their tsconfig.json.
mud/packages/dev-tools/package.json
Lines 12 to 15 in c8ffd09
"exports": { | |
".": "./dist/index.js" | |
}, | |
"types": "src/index.ts", |
With moduleResolution: node/node10, the types field is prioritized over the exports field.
Other templates, except for vanilla, already include @types/react-dom
as dependencies.
@@ -13,6 +13,8 @@ | |||
}, | |||
"devDependencies": { | |||
"@latticexyz/cli": "link:../../packages/cli", | |||
"@types/debug": "4.1.7", | |||
"@types/prettier": "2.7.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise here, I don't think we're using any of these packages directly, but getting this via the cli dep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cli is not relevant to this diff.
prettier and debug are added as dependencies at the workspace root because both contracts and client require them. Similarly, typescript is also a dependency in the workspace root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And both contracts and client use the TS files from common (via typesVersions), and common uses prettier and debug.
I looked into the |
I've created issue #2051 to explain why these |
Converted this to a draft because I think it needs a rebase after merging #2084 |
A rebase is not necessary because that PR doesn’t affect projects with However, we can remove these added |
I wonder if instead we should just move templates to use one of the other |
I think we can go ahead with merging this PR as a temporary fix until we get to step 2 & 3 from #2051 (releasing with |
Thank you for your reviews! I've now confirmed that a fresh project (
I believe the https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html |
tsc
errors in projects depending on@latticexyz/store-sync
#1974This pull request fixes TypeScript type errors occurring in projects created from the
create-mud
templates. Currently, users have to add some@types/*
dev dependencies to pass the tsc after creating their projects. To address this, I added the missing dependencies. Additionally, I created a GitHub Actions workflow that detects these and the errors fixed in the above PR.The added dependencies (
@types/debug
,@types/prettier
,@types/react-dom
) align with the versions of these packages used elsewhere in this repository. I considered adding them as non-dev dependencies to the origin libraries, such ascommon
, but they would be redundant for JavaScript projects, so I didn't take that approach.The workflow runs after a snapshot release. It uses the release to run
create-mud
andpnpm test
in the created projects, accurately simulating user environments. This is ensured by:/templates/
tests. This avoids missing errors related to non-included files or dependencies.node_modules
, potentially missing certain errors. For instance, the missing@types/prettier
error wouldn't have been detected if the test were run within/packages/create-mud/my-project
, as the repository root'snode_modules
contains@types/prettier
.Here are examples of this workflow's runs: This run lacked the type fixes from this PR and thus failed, while this run had the types and passed.
Initially, I started writing this test as a locally executable script for
pnpm test
. However, I realized that implementing it as a CI workflow would be simpler. So, I shifted to creating this workflow, but I'm open to the local approach if preferred. To replicate this locally, the script should build and pack all necessary packages in this repository, add them to the projects created bycreate-mud
outside of this repository, and then run the tests.