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

fix(templates): fix templates tsc errors and add test #2036

Merged
merged 7 commits into from Jan 3, 2024

Conversation

tash-2s
Copy link
Contributor

@tash-2s tash-2s commented Dec 7, 2023

This 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 as common, 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 and pnpm test in the created projects, accurately simulating user environments. This is ensured by:

  1. Not using locally linked dependencies, unlike the /templates/ tests. This avoids missing errors related to non-included files or dependencies.
  2. Running the test outside of this repository. Node.js/TypeScript might otherwise resolve modules/types using ancestor directories' 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's node_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 by create-mud outside of this repository, and then run the tests.

`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.
Copy link

changeset-bot bot commented Dec 7, 2023

⚠️ No Changeset found

Latest commit: c82181a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@@ -24,6 +24,7 @@
"viem": "1.14.0"
},
"devDependencies": {
"@types/react-dom": "18.2.7",
Copy link
Member

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?

Copy link
Contributor Author

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.

"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",
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@tash-2s
Copy link
Contributor Author

tash-2s commented Dec 9, 2023

I looked into the @types/* issue and will make an issue about it tomorrow. 😄

@tash-2s
Copy link
Contributor Author

tash-2s commented Dec 13, 2023

I've created issue #2051 to explain why these @types/* are necessary.
I think this test is beneficial because it helps to find errors related to packaging and dependencies, which are easy to overlook when working within the context of this repository. What are your thoughts on the workflow?

@holic holic marked this pull request as draft January 2, 2024 10:52
@holic
Copy link
Member

holic commented Jan 2, 2024

Converted this to a draft because I think it needs a rebase after merging #2084

@tash-2s
Copy link
Contributor Author

tash-2s commented Jan 2, 2024

A rebase is not necessary because that PR doesn’t affect projects with moduleResolution: node10 in their tsconfig, as mentioned in #2051 (comment). The templates use node10.

However, we can remove these added @types/ dependencies once we complete step 2 in the issue, so we can put this PR on hold. In the meantime, template users can run the pnpm add commands as needed.

@holic
Copy link
Member

holic commented Jan 3, 2024

I wonder if instead we should just move templates to use one of the other moduleResolution options?

@alvrs alvrs marked this pull request as ready for review January 3, 2024 17:35
@alvrs
Copy link
Member

alvrs commented Jan 3, 2024

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 publishConfig overrides, moving the moduleResolution to bundler, adding .d.ts.map files to keep devex of jumping to source files instead of d.ts files). Right now running pnpm test right after creating a fresh template fails, which might lead users to believe the templates are broken, so i think this temporary fix would be worth it. (And the new CI actions to ensure this doesn't happen in the future are useful as well)

@alvrs alvrs merged commit d3f0d32 into latticexyz:main Jan 3, 2024
10 of 11 checks passed
@tash-2s tash-2s deleted the test-and-fix branch January 3, 2024 18:18
@tash-2s
Copy link
Contributor Author

tash-2s commented Jan 3, 2024

Thank you for your reviews! I've now confirmed that a fresh project (pnpm create mud@main) set up on my local passed pnpm test, and the new CI has passed as well.

I wonder if instead we should just move templates to use one of the other moduleResolution options?

I believe the bundler config is recommended for most users, as they are likely to use bundlers (like vite or tsup) and bundler-like runners (such as tsx) with newer versions of Node.js. Therefore, updating the configs in the templates seems like a good idea. (However, we might want to wait for .d.ts.map?)

https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html

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.

None yet

3 participants