Skip to content

Conversation

@ndhoule
Copy link
Contributor

@ndhoule ndhoule commented Mar 7, 2025

This changeset enables typechecking on test files. It:

  • Separates the TypeScript configuration into two configurations:
    • tsconfig.json: Non-artifact-emitting configuration for development-time typechecking
    • tsconfig.build.json: Artifact-emitting configuration used to build the project
  • Updates tsconfig.json to run the type checker on test files
  • Adds a typechecking CI job
  • Fixes type errors in test files

@github-actions
Copy link

github-actions bot commented Mar 7, 2025

📊 Benchmark results

Comparing with 886c796

  • Dependency count: 1,182 (no change)
  • Package size: 281 MB (no change)
  • Number of ts-expect-error directives: 730 ⬆️ 0.82% increase vs. 886c796

@ndhoule ndhoule force-pushed the build/typecheck-more-files branch 6 times, most recently from 6618c34 to d873e25 Compare March 7, 2025 23:34
Copy link
Member

@serhalp serhalp left a comment

Choose a reason for hiding this comment

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

:grindog:

@ndhoule
Copy link
Contributor Author

ndhoule commented Mar 10, 2025

@serhalp Thanks for having a look! This PR isn't yet ready for review, but I'll tag you on it when it's done.

@ndhoule ndhoule force-pushed the build/typecheck-more-files branch 3 times, most recently from 3bedf7d to 3f6a850 Compare March 11, 2025 16:35
ndhoule added 6 commits March 11, 2025 09:36
This change separates the TypeScript configuration into two configurations:

- `tsconfig.json`: Non-artifact-emitting configuration for development-
  time typechecking
- `tsconfig.build.json`: Artifact-emitting configuration used to build
  the project

This includes no functional changes and is simply setup for future
typechecking improvements.
@ndhoule ndhoule force-pushed the build/typecheck-more-files branch from 3f6a850 to 54b8c13 Compare March 11, 2025 16:36
@ndhoule ndhoule marked this pull request as ready for review March 11, 2025 20:09
@ndhoule ndhoule requested a review from a team as a code owner March 11, 2025 20:09
Copy link
Member

@serhalp serhalp left a comment

Choose a reason for hiding this comment

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

👏🏼 Amazing, thanks for fixing this!

Comment on lines +1192 to +1193
// @ts-expect-error: We can't import Deno types without polluting the global environment
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
Copy link
Member

Choose a reason for hiding this comment

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

It looks like withEdgeFunction's handler arg also accepts a string, so another option would be to just pass this handler fixture as a raw string instead of pretending it's valid JS in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we should do this for all the fixtures--I'll do it in a separate PR when I get back. (Another reason to do it: you can currently reference in-scope variables but the test will end up failing.)

Most editors (and Prettier) will recognize code in javascript and typescript template tags and format the code accordingly, so I think that's a better alternative.

@ndhoule ndhoule merged commit 8ee88c9 into main Mar 11, 2025
52 checks passed
@ndhoule ndhoule deleted the build/typecheck-more-files branch March 11, 2025 21:45
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