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

The future of this fork/plugin #24

Closed
JounQin opened this issue Jan 17, 2024 · 45 comments
Closed

The future of this fork/plugin #24

JounQin opened this issue Jan 17, 2024 · 45 comments
Assignees

Comments

@JounQin
Copy link
Member

JounQin commented Jan 17, 2024

I'm thinking about maintaining this package as a complete independent plugin from upstream because a lot of issues can not be fixed easily without API changes, see

import-js#1479

import-js#2108

import-js#2111

import-js#2717

It would affect 3rd party resolvers like import-js/eslint-import-resolver-typescript#248

@SukkaW do you want to collaborate here?

@JounQin JounQin self-assigned this Jan 17, 2024
@JounQin JounQin pinned this issue Jan 17, 2024
@43081j
Copy link

43081j commented Jan 17, 2024

when i was considering forking the same plugin, i came to the conclusion it makes more sense to diverge than staying in sync.

there's a lot of extra logic, architecture, etc that could easily fall away these days IMO.

i'd rather you make it independent so you can be set free on it rather than working within constraints of the original plugin

just my two cents

if you need any help, too, i'd be happy to. i work on a fair amount of things in the eslint ecosystem and was about to do my own fork until i discovered this one!

@JounQin
Copy link
Member Author

JounQin commented Jan 17, 2024

Benefits

  1. No limitations anymore, I'll rewrite the whole package with TypeScript, Support for eslint-define-config #23 can be fixed as an effect, cc @Shinigami92
  2. Replace all the legacy codes to modern JavaScript/TypeScript
  3. Add features we want

Downsides

  1. Less contributors at the first place
  2. Maybe difficult to support both plugins, what's the future of https://github.com/import-js/eslint-import-resolver-typescript
  3. Per 2, should we support TypeScript as first-class, so basically we combined the two packages, is that worth or needed considering not everyone uses TypeScript and package size difference

@SukkaW
Copy link
Collaborator

SukkaW commented Jan 18, 2024

eslint-plugin-import has been missing a real update for years. We should take this opportunity to drop all Node.js <= 14 support.

@JounQin
Copy link
Member Author

JounQin commented Jan 18, 2024

eslint-plugin-import has been missing a real update for years. We should take this opportunity to drop all Node.js <= 14 support.

@SukkaW

Yes, we'll rewrite the whole code base into modern JavaScript/TypeScript features.

Do you want to join to maintain this project?

Next steps:

  1. I'll transfer this repository into @un-ts org because it will be changed as a TypeScript based library
  2. If you want to join, which level do you want to be involved, org level or repository level
  3. I'll start to setup the whole code base in a few days, the progress would just be similar as prettier/pretty-quick@v3.1.3...v4.0.0

@43081j
Copy link

43081j commented Jan 18, 2024

a few things i think could be cleaned up or thrown away too:

  • throw away memo-parser, feels like an experiment that got dumped in the repo and doesn't really belong here anymore
  • eslint-module-utils is a "package" that gets pulled from disk (utils/), seems unnecessary. probably good to just move this stuff into src/utils
  • check that those utils are actually needed
  • consider dropping the concept of a "resolver"

i don't believe there's any need for this resolver architecture anymore. feels like an over-complication of things, especially splitting them out into independent packages.

i think you should be able to use this plugin and it supports every day resolution out of the box (standard resolution browsers/node-esm use, and node resolution). anything else is an edge case you could support through config, but definitely don't need an extra package for.

if we use enhanced-resolve, that itself is configurable for all those cases. doesn't seem to be a reason to have resolvers anymore, just configure enhanced-resolve differently for those edge cases.

@JounQin
Copy link
Member Author

JounQin commented Jan 18, 2024

i think you should be able to use this plugin and it supports every day resolution out of the box (standard resolution browsers/node-esm use, and node resolution). anything else is an edge case you could support through config, but definitely don't need an extra package for.

if we use enhanced-resolve, that itself is configurable for all those cases. doesn't seem to be a reason to have resolvers anymore, just configure enhanced-resolve differently for those edge cases.

And that's why I'm using enhanced-resolve in https://github.com/import-js/eslint-import-resolver-typescript, I'll continue to use that package in the new core. While we still need to support tsconfig features like paths, allowArbitraryExtensions, customConditions and moduleSuffixes, etc.

I mean the new plugin will be a TypeScript-first plugin.

@Shinigami92
Copy link

Just want to leave my opinion here. I really like the idea of rewriting everything to TS and enhance the project in a good way.

Right now I don't feel like that I can over much time 🙁
But if you want, you can invite me into the org 🙂 and I can help when I have and find time

Another thing: I dislike the plugin-name "i", because it is not clear what "i" means. Is it possible to e.g. publish as @un-es/eslint-plugin-import or something like that?

@JounQin
Copy link
Member Author

JounQin commented Jan 18, 2024

The idea of eslint-plugin-i comes from eslint-plugin-n.

Personally, I don't think we should rename to a new brand considering it's already a little bit well known.

@Shinigami92
Copy link

The idea of eslint-plugin-i comes from eslint-plugin-n.

Personally, I don't think we should rename to a new brand considering it's already a little bit well known.

I also hate n, what does "n" stand for? I would need to look this up in the Readme or Repo...

@JounQin
Copy link
Member Author

JounQin commented Jan 18, 2024

I also hate n, what does "n" stand for? I would need to look this up in the Readme or Repo...

I don't think anyone would use @un-es/eslint-plugin-import before knowing this is the active more elegant fork, I may prefer to use the non-fork version eslint-plugin-import.

n or i, on another hand, indicates that it's much lightweight somehow. If you know what it does at once, you'll never forget it.

@ljharb
Copy link

ljharb commented Jan 18, 2024

To clarify, eslint-module-utils is a separate package; the repo is a kind of hybrid monorepo. You’re right about the memo parser, that isn’t used.

A great many users need the webpack resolver, or use the plugin with ecosystems that don’t use standard node resolution, so just be aware of who’s being excluded if you choose to drop the concept of resolvers.

@43081j
Copy link

43081j commented Jan 18, 2024

i don't believe we need a resolver architecture/plugin system for that

nothing is stopping us providing configurable resolution using a single built-in resolver. i think it was an over-engineering of a much simpler thing. enhanced-resolve plus some configuration will achieve everything we need.

just my opinion though. sure you believed the opposite when you built it, which is fair enough. each to their own

@ljharb
Copy link

ljharb commented Jan 18, 2024

I didn’t build it.

@JounQin
Copy link
Member Author

JounQin commented Jan 18, 2024

enhanced-resolve is good enough to handle webpack things. eslint-module-utils is needless anymore.

@JounQin
Copy link
Member Author

JounQin commented Jan 18, 2024

I've started the migration at #26

@fisker
Copy link

fisker commented Jan 18, 2024

Really glad to see someone start fork it, I had really bad experience contribute to the original repo once, and decide never do it again..

I dislike the plugin-name "i".

Same here.

@JounQin
Copy link
Member Author

JounQin commented Jan 19, 2024

Progress: I've successfully removed all resolver concepts by using enhanced-resolve directly at f747071 (#26) as @43081j proposed. And eslint . run in this project itself is just working, the next step will be focusing on testing.

@JounQin
Copy link
Member Author

JounQin commented Jan 19, 2024

@fisker @Shinigami92

Do you think @eslinter/eslint-plugin-import or @eslint-community/eslint-plugin-import would be accepted?

I own @eslinter but I don't know how to join @eslint-community, and eslint-community is a bit too long to myself, like @typescript-eslint, I'd prefer @tseslint for example.

Although it's too late as @bradzacher said, but we still have the chance.


Another interesting thing is that, I own https://github.com/eslinter/eslint-plugin-jsx and the npm package now, so I'll start to adapt @jsx-eslint packages like https://github.com/jsx-eslint/eslint-plugin-react into eslint-plugin-jsx when I finished the migration of eslint-plugin-import.

I really hope @Rel1cx can join in this case.

@43081j
Copy link

43081j commented Jan 19, 2024

doing such good work already 🙏 ill have a read through the PR this weekend if i can

on the naming - i do agree it'd be nice if it had a "full" name (rather than i) but i understand it already has a fair amount of users. if it were upto me, i'd probably call it eslint-plugin-import under a scope.

the eslint community org would be good to put this into but you're right that the scope is pretty lengthy. even if you don't adopt that name, it may still be worth joining that org on github though (assuming it is "official" and actively managed, and doesn't remove any ownership from you).

@fisker
Copy link

fisker commented Jan 19, 2024

Any chance to have eslint-plugin-module or eslint-plugin-modules?

@JounQin
Copy link
Member Author

JounQin commented Jan 19, 2024

Any chance to have eslint-plugin-module or eslint-plugin-modules?

@fisker I'll try to contact these two package owners.

@Shinigami92
Copy link

@eslint-community/eslint-plugin-import would be nice, @eslint-community is a trusted community group

@fisker
Copy link

fisker commented Jan 19, 2024

Does @eslint-community/eslint-plugin-import even work in legacy ESLint config?

It should be eslint-plugin-___ or @___/eslint-plugin, am I wrong? If it works, the plugin name will be @eslint-community/import?

@JounQin
Copy link
Member Author

JounQin commented Jan 19, 2024

Does @eslint-community/eslint-plugin-import even work in legacy ESLint config?

Yes. The users can always use the plugin full name or @eslint-community/import for shorthand.

@fisker
Copy link

fisker commented Jan 19, 2024

{
   '@eslint-community/import/consistent-type-specifier-style': 'off'
}

Great rule name! 😄

@43081j
Copy link

43081j commented Jan 20, 2024

I'll migrate eslnt-plugin-react in next step.

did you already start this? i have a branch lying around which does a bunch of it from what i remember

if you want any help, ping me when you make the repo and i'll push the branch for you to look at, at least

(don't want to take this thread off topic so lets catch up once the repo exists rather than here)

@ljharb
Copy link

ljharb commented Jan 21, 2024

@JounQin true, because that makes it useless for preact and svelte and vue and styled-components etc users.

@JounQin
Copy link
Member Author

JounQin commented Jan 21, 2024

@43081j

I haven't personally, but I know a great project https://github.com/Rel1cx/eslint-react, that's why I said I really hope @Rel1cx can join us.

@Rel1cx
Copy link

Rel1cx commented Jan 21, 2024

@43081j

I haven't personally, but I know a great project Rel1cx/eslint-react, that's why I said I really hope @Rel1cx can join us.

I strongly support you to do a full rewrite of eslint-plugin-import, maybe called eslint-plugin-esm + eslint-plugin-cjs (two separate packages), or eslint-plugin-module (one package).
But as for migrating eslint-plugin-react to eslint-plugin-jsx, my point of view is consistent with this reply Rel1cx/eslint-react#52 (comment).

@JounQin
Copy link
Member Author

JounQin commented Jan 21, 2024

@Rel1cx In my view, some rules are universal, some rules are framework specific.

For react rules, we can have jsx/react/x rules, for vue rules, we can have jsx/vue/x rules, etc, and we can have jsx/x rules for universal cases.

So in eslint-plugin-jsx, @eslint-react could be an dependency.

@Rel1cx
Copy link

Rel1cx commented Jan 21, 2024

You could make an eslint-plugin-jsx but that only makes sense if the rules in it are truly framework-agnostic, and considering that most of the natural framework-agnostic rules have already been migrated to @stylistic/eslint-plugin-jsx, there aren't that many left.

Where JSX fits into the ecosystem of front-end languages and frameworks:

JS in Vue
TS in Vue
TSX in Vue

JS in Solid
TS in Solid
TSX in Solid

...

In essence, JSX is just a syntax extension, how to lint TSX here is the same as TS, JS here, it's up to the frameworks, making an eslint-plugin-jsx that doesn't just take care of the correctness of JSX itself, but also has to be configured (or prefixed) to support the behaviour of all the specific frameworks that have used JSX in the past, are using JSX now, and will be in the future is equivalent to making a @typescript-eslint/eslint-plugin but not only for the correctness of TS itself but also configured (or prefixed) to support the behaviour of all the frameworks that use TS, React, Preact, Solid, Vue, etc.

Don't think of JSX as anything special, it's basically the same thing as JS and TS.

@43081j
Copy link

43081j commented Jan 21, 2024

Just my two cents, I roughly agree.

  • there should be a jsx plugin which lints jsx syntax (similar to a html validator), but nothing framework specific
  • there should be a plugin for each framework with the framework specific rules, whether using jsx or not

@JounQin
Copy link
Member Author

JounQin commented Jan 21, 2024

OK, like @jsx-eslint, I own @eslint-jsx, we can make framework specific plugins at that org.

@JounQin
Copy link
Member Author

JounQin commented Jan 21, 2024

Progress:

Test Suites: 21 failed, 33 passed, 54 total
Tests:       171 failed, 2108 passed, 2279 total

@JounQin
Copy link
Member Author

JounQin commented Feb 6, 2024

After thinking, I think eslint-plugin-import-x could be the best choice just like https://github.com/eslint-community/eslint-plugin-es-x.

@43081j
Copy link

43081j commented Feb 6, 2024

i think that makes sense, since future forks of other plugins could follow the same naming convention too. nice to have some consistency there

@JounQin
Copy link
Member Author

JounQin commented Mar 12, 2024

I've published eslint-plugin-import-x as the successor of eslint-plugin-i, it should work as previous for now, except it's not an alias anymore, all settings are renamed into import-x/ prefix.

The whole progress was tough, #26 was unable to continue because I chose to migrate codes instead tests first.

But finally I migrated babel-core into @babel/core, babel-eslint into @babel/eslint-parser, etc., successfully at #30. And I also migrated mocha/sinon to jest.

But I haven't removed the resolver concept: let's continue discussing at #40.


In the next step, I'll try to migrate the whole code base into TypeScript.

@JounQin JounQin closed this as completed Mar 12, 2024
n0099 added a commit to n0099/open-tbm that referenced this issue Jun 5, 2024
$ yarn add -D eslint-plugin-import-x # due to un-ts/eslint-plugin-import-x#24
$ sed -i "s@(:|')import/@\1import-x/@" .eslintrc.cjs
@ fe
n0099 added a commit to n0099/open-tbm that referenced this issue Jun 5, 2024
$ yarn add -D eslint-plugin-import-x # due to un-ts/eslint-plugin-import-x#24
$ git ls-files -z | xargs -0 sed -i "s@(:|'| )import/@\1import-x/@"
@ fe
n0099 added a commit to n0099/open-tbm that referenced this issue Jun 5, 2024
$ yarn remove eslint-plugin-import
$ git ls-files -z | xargs -0 sed -i "s@(:|'| )import/@\1import-x/@"
@ fe
theoludwig added a commit to theoludwig/eslint-config-conventions that referenced this issue Jul 10, 2024
We'll consider using eslint-plugin-import-x in the next major version,
#3
We remove this plugin to avoid issues like this: pnpm/pnpm#4619
(should be resolved when ESLint v9 is fully supported)
And because of the lack of features, that eslint-plugin-import-x implement:
(un-ts/eslint-plugin-import-x#24 (comment))
meyfa added a commit to meyfa/eslint-config that referenced this issue Aug 3, 2024
See un-ts/eslint-plugin-import-x#24, and the
references therein, for context.

This is also required to support ESLint 9.
meyfa added a commit to meyfa/eslint-config that referenced this issue Aug 3, 2024
See un-ts/eslint-plugin-import-x#24, and the
references therein, for context.

This is also required to support ESLint 9.
meyfa added a commit to meyfa/eslint-config that referenced this issue Aug 3, 2024
See un-ts/eslint-plugin-import-x#24, and the
references therein, for context.

This is also required to support ESLint 9.
meyfa added a commit to meyfa/eslint-config that referenced this issue Aug 3, 2024
See un-ts/eslint-plugin-import-x#24, and the
references therein, for context.

This is also required to support ESLint 9.
meyfa added a commit to meyfa/eslint-config that referenced this issue Aug 3, 2024
See un-ts/eslint-plugin-import-x#24, and the
references therein, for context.

This is also required to support ESLint 9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants