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: support TypeScript config using importx #18440

Closed
wants to merge 32 commits into from

Conversation

antfu
Copy link
Contributor

@antfu antfu commented May 10, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Support loading eslint.config.ts eslint.config.mts eslint.config.cts files powered by importx. The support is opt-in by users, where they need to install importx explicitly (we include it as optional peer deps instead of dependencies)

Based on my previous experiments with eslint-ts-patch(which supports swapping different loaders like jiti tsx bundle-require, feel free to try them out). importx is a package trying to ease out the differences between them and the complexity of the pros/cons hidden from ESLint.

The only downside is that tsx uses Node's API, which has only been available since v20.8.0 and v18.19.0. But considering this is an opt-in feature, it should be fine, and the ecosystem should catch up soon. importx ease out that will auto fallback to jiti on unsupported node version.

Is there anything you'd like reviewers to focus on?

@antfu antfu requested a review from a team as a code owner May 10, 2024 21:23
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label May 10, 2024
@github-actions github-actions bot added cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features labels May 10, 2024
Copy link

netlify bot commented May 10, 2024

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 83c3170
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/668007ec469f5f0008c45bb7
😎 Deploy Preview https://deploy-preview-18440--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Co-authored-by: aryaemami59 <aryaemami59@users.noreply.github.com>
@fasttime fasttime added the needs design Important details about this change need to be discussed label May 11, 2024
@nzakas nzakas marked this pull request as draft May 13, 2024 14:39
@nzakas
Copy link
Member

nzakas commented May 13, 2024

Marking as a draft to avoid accidental merging, as there is an RFC being discussed.

Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@antfu antfu changed the title feat: support TypeScript config using tsx feat: support TypeScript config using importx May 24, 2024
@fasttime fasttime added accepted There is consensus among the team that this change meets the criteria for inclusion and removed needs design Important details about this change need to be discussed labels May 27, 2024
@fasttime
Copy link
Member

Thanks for this PR, @antfu! In order to move forward with this pull request, we will need some unit tests to verify that TypeScript config files are being loaded as expected.

My suggestion would be to start looking at these tests and add similar tests to check the behavior with packages that have config files with .ts, .cts and .mts extension in place of .js, .cjs and .mjs respectively. It would be also useful to have a test that checks loading a config file whose path is specified in overrideConfigFile like this one. When you are done, just mark the PR as ready for review. And of course, just ping me if you need help!

@antfu
Copy link
Contributor Author

antfu commented Jun 29, 2024

Sorry for the long due - with the great help from @privatenumber on tsx, now all the tests are passing and should be good to go!

I will further make sure everything works well in the future on importx side.

@antfu antfu marked this pull request as ready for review June 29, 2024 13:03
@nzakas
Copy link
Member

nzakas commented Jul 1, 2024

Thanks. We still need to decide between this approach and #18134. @aryaemami59 what do you think of this PR?

Also, we'll need this functionality to be behind a feature flag (unstable_ts_config). You can add the flag in https://github.com/eslint/eslint/blob/main/lib/shared/flags.js to activeFlags and then use eslint.hasFlag() to determine if the flag is present to enable this functionality.

@aryaemami59
Copy link

@nzakas I haven't worked with importx so I'm not entirely sure but from what I can tell it looks good to me.

@nzakas
Copy link
Member

nzakas commented Jul 3, 2024

@antfu @aryaemami59 @fasttime I'd just like everyone to get on the same page before two people keep building the same feature.

What is the key decision here? Jiti vs importx?

How can we make that decision?

@fasttime
Copy link
Member

fasttime commented Jul 4, 2024

What is the key decision here? Jiti vs importx?

How can we make that decision?

It looks like Jiti 2 would support a similar set of test cases as importx, so the choice of the underlying tool seems to be more an implementation detail than a feature at this point. I wouldn't be opposed to merging both PRs and updating the logic to use whichever tool is available (if importx is installed, use it; otherwise use Jiti 2 if it's installed). Then maybe we could get user feedback about both tools, and we could always simplify the logic later to use just one tool for easier maintenance. But that's just an option. I'm fine with having just one implementation if there are any advantages.

@aryaemami59
Copy link

@nzakas I'm obviously biased but I think we should go with jiti. It's probably best if we wait till version 2 is stable, we can do without it but top-level await wouldn't be supported. I think part of the reason I like jiti as an option is we don't really have to worry about module resolution all that much. They can write CJS code in a .mts file or ESM code in a .cts file. Not only that, even within the same file, they can use both ESM and CJS syntax (I wrote tests on this recently). So in one file you can have something like this:

import { someConfig } from 'somePackage';

module.exports = someConfig;

Or something like this:

const { someConfig } = require('somePackage');

export default someConfig;

It also doesn't care at all about tsconfig.json so that's another thing we won't have to worry about. It just makes everything easier and that's what I like about it.

@christopher-buss
Copy link

christopher-buss commented Jul 4, 2024

I personally have no stake in either implementation, but importx already uses jiti under the hood where required, so it seems strange to me merge both a tool that uses jiti, or jiti directly. This is in response to @fasttime floating the idea of merging both tools.

It'd make more sense to pick one or the other rather than allowing both. If jiti provides everything required then a tool like importx shouldn't be needed as a middleman. If it doesn't offer enough flexibility and importx can cover those shortfalls, then that would make more sense as a choice.

The advantage I can see to importx (today) is that as antfu mentioned, once jiti v2 comes out of beta, importx can upgrade to it internally without the need for any changes on the eslint side. The issue with jiti v2 today is that we're unlikely to support jiti directly until v2 comes out officially, and there's no hard-eta on that. importx would allow these changes to (likely) go through sooner, unless eslint is willing to support technology that is not currently officialy released.

@nzakas
Copy link
Member

nzakas commented Jul 4, 2024

@aryaemami59 my understanding is that importx works exactly the same way, especially since it uses jiti underneath. So I'm not sure you've differentiated the two approaches.

@fasttime we should pick one to start. I don't see any benefit to making people choose a transitive dependency to try this out.

The advantage I can see to importx (today) is that as antfu mentioned, once jiti v2 comes out of beta, importx can upgrade to it internally without the need for any changes on the eslint side.

This doesn't seem like any real, tangible advantage to me. Either we directly update jiti or we need to update importx in order to get an updated jiti.

My opinion: I think we should go with jiti for the initial feature. It's been around a while, and although it doesn't support top-level await, I don't think that's a blocker for an experimental feature. importx is currently two months old, so still very much experimental, and even though I respect @antfu's work greatly, we do need to be careful when including young dependencies into a product used by so many people.

@aladdin-add
Copy link
Member

The advantage of importx is: it automatically selects the appropriate loader and not necessarily jiti - https://github.com/antfu-collective/importx?tab=readme-ov-file#loaders

So if the user is already using one of the ts compilers (e.g. tsx), the user does not need to install another jiti.

@fasttime
Copy link
Member

fasttime commented Jul 5, 2024

My opinion: I think we should go with jiti for the initial feature. It's been around a while, and although it doesn't support top-level await, I don't think that's a blocker for an experimental feature. importx is currently two months old, so still very much experimental, and even though I respect @antfu's work greatly, we do need to be careful when including young dependencies into a product used by so many people.

@nzakas The other PR #18134 was updated to use Jiti 2 beta which does support top-level await. The roadmap issue for Jiti v2 is already closed, and it suggests that Jiti 2 stable is nearly ready. Do you recommend to revert to Jiti 1 stable or should we stay with Jiti 2?

@nzakas
Copy link
Member

nzakas commented Jul 5, 2024

@aladdin-add thanks for explaining. Still, I don't think that's a big enough difference.

@fasttime if jiti v2 is close, I'd say let's wait until next week and see where we are.

@privatenumber
Copy link
Sponsor

privatenumber commented Jul 7, 2024

@nzakas

importx is currently two months old, so still very much experimental, [...] we do need to be careful when including young dependencies into a product used by so many people.

Can't the same argument be used against a major release of Jiti v2?


I'm still interested in seeing if we can respect the user's TS compiler preference.

Like many agreed in the RFC, I wouldn't want to be forced to install a second TS compiler if I already have one (especially by such a popular tool). And being able to choose would support users who have use-cases for TLA or native Node integration.

Could we try a simple fallback approach like postcss-load-config?

@nzakas
Copy link
Member

nzakas commented Jul 8, 2024

@privatenumber This is an experimental feature, so we can continue to iterate. I'm mostly interested in landing the feature so we can start having people try it out and give us feedback. It doesn't have to be perfect, but I prefer starting with a package that has some lineage.

@privatenumber
Copy link
Sponsor

I can get onboard with iterating. Would you mind elaborating on what you mean by "lineage" in this context?

@fasttime
Copy link
Member

Hi all 👋 It's been a wish of the team to have only one PR to implement RFC eslint/rfcs#117. It wasn't clear from the beginning how the implementations with different tools would work, but now that there are well-tested prototypes, it's better to concentrate all efforts in the same direction.

We would like to keep #18134 because it was created first and it is from the author of the RFC. Of course, we are aware of the mutual exchange of ideas and code that's been going on so far, and we would like to give credit to @antfu for contributing to the implementation. Thanks also to everybody who provided feedback and helped reviewing this PR.

As for the tool to use, the current preference is for Jiti, which we hope to get set up in version 2 soon. This is a choice that we may reconsider later depending on the feedback we receive when the feature is out. Please, take time to review #18134 if you would like to contribute to adding TypeScript config support to ESLint.

@fasttime fasttime closed this Jul 10, 2024
@aryaemami59
Copy link

@privatenumber

Could we try a simple fallback approach like postcss-load-config?

I really like that idea, we can check for tsx and fallback to jiti. I can go ahead and put together the implementation on a separate branch.

@acidoxee
Copy link

we can check for tsx and fallback to jiti

I'm curious, isn't that more or less what importx already does? It seems they use a mix of techniques to detect the runtime's compatibility with all tools, among which are dynamic import checks and ambiant Node versions. Isn't this direction pointing towards what importx already strives to perform, being kind of a "router pointing to the proper tool"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

9 participants