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

feat: full ESM config support using Jiti #1041

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

henrist
Copy link

@henrist henrist commented Mar 10, 2025

I hereby confirm that I followed the code guidelines found at engineering guidelines <-- I don't have access to this

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Resolves #1015

The current use of ts-node is problematic:

  • Only CommonJS is supported, so loading ESM doesn't work as expected
  • ts-node is currently not maintained and haven't received updates for 1+ year. It lacks newer TypeScript features. For instance it does not support multiple extends in tsconfig.json that came with TypeScript 5.

This has been a large pain as most of the code I work with is in ESM. It has required non-trivial tsconfig files to workaround this.

This PR introduces Jiti as an alternative to ts-node. I could have picked tsx as well, but I don't find the API of tsx as intuitive/simple as Jiti. This is very similar as the implementation of eslint that uses Jiti for its TypeScript support.

Additional file extensions are also supported: .mts, .cts, .cjs

To avoid a breaking change it supports both ts-node and jiti. Existing users with only ts-node should not be affected, while new users will be recommended to add jiti. Jiti does not depend on a tsconfig.json file (and does not do type checking), so typescript is not requested as a dependency. Existing users can add jiti to get better (and ESM) support, and we also recommend this on compile issues. If you're open for a breaking change we could remove ts-node support and avoid dealing with both.

@sorccu
Copy link
Contributor

sorccu commented Mar 11, 2025

Hi @henrist,

I really like the change, but I will need some time to consider any possible backwards compatibility implications caused it. I may not be able to properly review this until next week (it's the end of tax season over here which limits my extra time), but based on a quick scroll through I think it looks great overall. Backwards compatibility and potential breakage is my main concern and I'm grateful that you seem to have kept that in mind. For now, we are not ready to fully remove ts-node but that may be a possibility in a future major version.

Do note that while this PR may add full ESM support on the CLI side, our runtimes do not yet support ESM due to complexities of the current architecture, so unfortunately this PR does not fully enable ESM on the entire Checkly platform. However, we have a project in progress to allow ESM to work on our runners, so ESM everywhere may soon be a possibility too. For now though it's a limitation that you have to keep in mind.

Thanks a lot!

@sorccu sorccu self-assigned this Mar 11, 2025
@henrist
Copy link
Author

henrist commented Mar 11, 2025

@sorccu Thanks for your quick feedback! Looking forward to your review. I have put some effort into avoiding this to become a breaking change. I've tested it an original template project, new template project and on a couple of our repos. But useful to have someone else double check.

Great to hear that you're working on ESM support on the runner side as well, although due to the limitation of dependencies and the fact that it parses module syntax just fine, this haven't been a real problem for us. I did use the term "config" in the PR title in an attempt to make it clear that this is for config of checks, not the check scripts/specs themself.

Would it be possible to trigger the canary/experimental release for this PR?

@sorccu
Copy link
Contributor

sorccu commented Mar 12, 2025

I would be happy to trigger a build, but unfortunately that will also have to wait till next week as our workflows do not currently give access to secrets for PRs from forks, which means that both integration tests and the build would fail. I will massage the workflows a little next week to make that work.

@sorccu sorccu self-requested a review March 12, 2025 09:03
@sorccu
Copy link
Contributor

sorccu commented Mar 24, 2025

Unfortunately I was extra busy last week and did not have bandwidth to test your PR, but I will handle this within the next 2 days. Super excited about this feature.

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.

bug: The 'import.meta' meta-property is only allowed when the...
2 participants