Skip to content

feat: migrate to import-x with lighter deps and better performance #129

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

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

Conversation

JounQin
Copy link

@JounQin JounQin commented Mar 29, 2025

@acelaya
Copy link
Member

acelaya commented Mar 29, 2025

Before moving away from a well established plugin for a fork, could you give me a bit more context?

I understand it says import-x is lighter and faster, but could you explain how?

Ultimately, can you share the discussions that motivated you guys to create this fork, and why was it not possible to achieve the same in the original project?

@JounQin
Copy link
Author

JounQin commented Mar 29, 2025

@acelaya See also un-ts/eslint-plugin-import-x#24, un-ts/eslint-plugin-import-x#40, un-ts/eslint-plugin-import-x#208

eslint-plugin-import aims supporting very outdated Node 4 with very outdated dependencies, we don't.

@acelaya
Copy link
Member

acelaya commented Mar 30, 2025

Thanks! I'll take a look at those threads, but a couple more questions.

If import-x is supposed to be faster, are there any benchmarks published somewhere that support this statement?

If import-x is supposed to have a lighter dependency footprint, how come this PR is adding a bunch of new dependencies when it is removing eslint-plugin-import?

When you say eslint-plugin-import aims supporting very outdated Node 4 with very outdated dependencies, is this something they have stated as a conscious decision, or simply a lack of time of their maintainers? And if it's the later, and seeing you have contributed to that repository as well, why was a fork the best solution over improving the original library? Were the main maintainers against introducing changes?

And ultimately, would this have helped on the issue you helped me solve, for which you provided this PR? shlinkio/shlink.io#923. I guess un-ts/eslint-plugin-import-x#208 partially answers this question.

@JounQin
Copy link
Author

JounQin commented Mar 30, 2025

If import-x is supposed to be faster, are there any benchmarks published somewhere that support this statement?

We haven't add such benchmarks, some theory proof (we'd add benchmarks indeed):

  • short dependency depths
  • plugin-import uses tsconfig-paths + typescript itself to load tsconfig, plugin-import-x uses get-tsconfig
  • plugin-import uses resolve which doesn't support exports, plugin-import-x uses unrs-resolver which is a napi-rs project
  • plugin-import-x's v3 interface shares single resolver like my PR createTypeScriptImportResolver() which is used all across resolving chains

But the way, you can just try TIMING=1 npm run lint before and after to try out.

If import-x is supposed to have a lighter dependency footprint, how come this PR is adding a bunch of new dependencies when it is removing eslint-plugin-import?

Because you have other plugins also maintained by eslint-plugin-import's maintainer, that results same deep dependencies, which can be replaced by other replacements like:

And also https://github.com/SukkaW/nolyfill

Were the main maintainers against introducing changes?

Well, see

And ultimately, would this have helped on the issue you helped me solve, for which you provided this PR?

Not sure to understand, your previous has already been resolved without migrating, this PR aims performance.

@JounQin
Copy link
Author

JounQin commented Mar 30, 2025

There in no lint script in this repo, so I can not run the benchmark for you here.

@acelaya
Copy link
Member

acelaya commented Mar 30, 2025

Thanks! That's a super detailed explanation. Now I need some time to go over those threads.

Also, I want to test the changes in downstream projects (this is a shared configuration I use in a bunch of places), before making a decision.

Because you have other plugins also maintained by eslint-plugin-import's maintainer

I suppose with this you mean that removing eslint-plugin-importer does not remove any of its dependencies because other plugins still depend on them. That makes sense.

I suppose, if this was merged, I would want to migrate those as well, to their more lightweight alternatives.

And ultimately, would this have helped on the issue you helped me solve, for which you provided this PR?

Not sure to understand, your previous has already been resolved without migrating, this PR aims performance.

Yeah, sorry, that was a bit vague.

What I meant is that, in that PR you provided some extra eslint config that was aiming to solve some importer-related issues, when used in combination with astro files.

My question is, if using import-x would have made that process simpler in any way?

At least I see configuring the resolvers seems a bit more straightforward here.

@JounQin
Copy link
Author

JounQin commented Mar 30, 2025

My question is, if using import-x would have made that process simpler in any way?

That doesn't change the parsers concept for parsing modules, so those changes are still needed.

We may change default languageOptions as module + ecmaVersion: latest in the future.

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.

2 participants