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

What breaking changes should be included in the next major version (v5) release? #235

Open
SukkaW opened this issue Mar 11, 2025 · 15 comments

Comments

@SukkaW
Copy link
Collaborator

SukkaW commented Mar 11, 2025

  • Drop Node.js 18 supports
    • This allows eslint-plugin-import-x to become ESM-only
    • If we only target Node.js 20+, then require(ESM) will be available in eslint.config.cjs.
  • Make import resolver interface v3 default for import-x/resolver
    • Resolvers using the v1 and v2 interface can be wrapped within our built-in compat utility, or be configured using the import-x/resolver-legacy options (which will be removed in v6 I guess?).
    • With the v3 interface, it is way easier for custom resolvers to share instances across invocations, significantly improving the performance, see feat: implement custom resolver interface v3 #192
  • Drop eslint-import-resolver-node with our built-in Replacing the built-in default import resolver that supports exports and imports from package.json #208 feat: implement a resolver that supports exports #209
    • Technically, this is not a breaking change since the resolve behavior will not change. But we support imports and exports maps. This is because ljharb refuses to implement such features in eslint-import-resolver-node and even locks the related issue.
@JounQin
Copy link
Member

JounQin commented Mar 11, 2025

Drop Node.js 18 supports

I don't see quite benefits to become ESM only for a widely used ESLint plugin at this time

Make import resolver interface v3 default for import-x/resolver

I forget to suggest use extra options instead of resolver as described at import-js#2108 (comment) and import-js#2519

I don't know whether would it be possible to support it without breaking changes

Drop eslint-import-resolver-node with our built-in

Great to replace enhanced-resolver with oxc-resolver inside.

I'll investigate import.meta.resolve solution failing cases in the next few days.

@SukkaW
Copy link
Collaborator Author

SukkaW commented Mar 11, 2025

I don't see quite benefits to become ESM only for a widely used ESLint plugin at this time

The idea is that the major version bump is already coming, and we might just migrate to ESM-only considering that Node.js 20+ supports require(ESM) now, in response to the movement: https://antfu.me/posts/move-on-to-esm-only.

Also, @43081j can help us investigate and migrate to ESM-only.

I don't know whether would it be possible to support it without breaking changes

It is already possible with interface v3, see this example:

// eslint.config.js
'import-x-/resolver': [
  createXResolver({} /** options */)
]

// resolver.js
export function createXResolver(options) {
  // I can do something with options here
  const reusableInstance = new Factory(options);

  return {
    interfaceVersion: 3,
    name: 'x',
    resolve(src, mod) {
      // I can still access options even down here
      reusableInstance.method(src, mod, options);
    }
  }
}

Basically, you can access options anywhere in the scope, including the resolve function.

Great to replace enhanced-resolver with oxc-resolver inside.

oxc-resolver is just a RIIR version of enhanced-resolve plus the tsconfig.json reading and way better performance. We probably should include this in the major version bump.

@JounQin
Copy link
Member

JounQin commented Mar 11, 2025

The idea is that the major version bump is already coming, and we might just migrate to ESM-only considering that Node.js 20+ supports require(ESM) now, in response to the movement: antfu.me/posts/move-on-to-esm-only.

I understand the we want ESM eventually, but as a downstream plugin for ESLint, I would recommend follow ESLint's strategy unless there are huge features we must implement in ESM, otherwise cjs consumers are blocked without any benefits

It is already possible with interface v3, see this example

What I'm linking is not about resolver's options but import plugin's running context, context.cwd in this case, see aslo import-js/eslint-import-resolver-typescript#74 (comment)

oxc-resolver is just a RIIR version of enhanced-resolve plus the tsconfig.json reading and way better performance. We probably should include this in the major version bump.

Yes, should we enter next release now?

https://github.com/changesets/changesets/blob/main/docs/prereleases.md#prereleases

@43081j
Copy link

43081j commented Mar 11, 2025

I would recommend follow ESLint's strategy

What strategy is that?

All plugins are loaded by a dynamic import inside eslint, relying on node to deal with cjs interop. Moving to esm would mean you can't use this plugin in a cjs config file before node 20 basically, but nothing else would change.

If we come back in a year, I suspect you'll be given the same decision to make. There will always be someone with an old cjs config somewhere

@JounQin
Copy link
Member

JounQin commented Mar 11, 2025

What strategy is that?

I personally would prefer following https://github.com/eslint/eslint/blob/c99db89141f1601abe6f9d398a4b6c126e3a0bdb/package.json#L6 and https://github.com/eslint/eslint/blob/c99db89141f1601abe6f9d398a4b6c126e3a0bdb/package.json#L223 or dual packaging for now.

But we can made a prototype to seem if we migrate to ESM only, how much would the size be reduced, say we have 5.4MB / 54 packages and eslint-plugin-import has 6.8MB / 120 packages now. (It seems our dependencies are large because of TypeScript related tool chains)

And also we should do some analysis on our dependents.

@43081j
Copy link

43081j commented Mar 11, 2025

The engine constraint isn't affected. You can continue to use node 18 if this goes esm only

It just means your config should be esm too

eslint can't really be compared to the plugins either. It'll be commonjs for much longer than plugins are because it's a much bigger job to rework it into esm

We certainly shouldn't move to a dual package in any situation. Either we should stick with cjs and move to esm later, or move to esm now. One way or another, the move would happen and before eslint itself moves

@JounQin
Copy link
Member

JounQin commented Mar 11, 2025

Just check some dependents randomly, almost all of them are using ESM config file, so let's go for ESM only. 🍻

@SukkaW
Copy link
Collaborator Author

SukkaW commented Mar 11, 2025

otherwise cjs consumers are blocked without any benefits

Node.js 20+ has require(ESM) support. So there won't be any blockers TBH. The only blocker would be the Node.js 20+ requirements for those using CJS with their ESLint config.

@43081j
Copy link

43081j commented Mar 12, 2025

That's true. Seems we're all in agreement 🥳

It is scary times making such a big decision but I think we can set a good example here for others to follow. If we can figure it out, it'd be really great

@SukkaW
Copy link
Collaborator Author

SukkaW commented Mar 12, 2025

That's true. Seems we're all in agreement 🥳

It is scary times making such a big decision but I think we can set a good example here for others to follow. If we can figure it out, it'd be really great

But if we don't raise the engine constraints, we will have to write a detailed docs explaining:

  • We recommend upgrading to Node.js 20+ so you can both require and import eslint-plugin-import-x
  • If you need to stick with Node.js 18, you will have to use eslint.config.mjs, but it is OK, the rest of your app can still be CJS.

If we raise the engine constraints to 20+, then there will be no changes needed toward eslint.config.cjs, only upgrading Node.js.

@JounQin @43081j So what do you think, should we raise the engine constraints or not?

@JounQin
Copy link
Member

JounQin commented Mar 12, 2025

No need to raise to 20+, eslint.config.mjs requirement for Node 18 users is already fine enough.

@43081j
Copy link

43081j commented Mar 13, 2025

I think as long as we document that you should use an mjs config in <20 , we don't need to bump the engine constraint

We may not need to document the fact you can require/import it in 20+ since they shouldn't need to care (as it'll work as is)

@JounQin
Copy link
Member

JounQin commented Mar 16, 2025

I'd like to change this repo a mono repo by adding a new package like eslint-import-utils for sharing some logic for reusing in 3rd-party resolvers, some kind of https://www.npmjs.com/package/eslint-module-utils, what do you think? @SukkaW

@SukkaW
Copy link
Collaborator Author

SukkaW commented Mar 17, 2025

I'd like to change this repo a mono repo by adding a new package like eslint-import-utils for sharing some logic for reusing in 3rd-party resolvers, some kind of https://www.npmjs.com/package/eslint-module-utils, what do you think? @SukkaW

Sure! And let's use the turborepo.

@JounQin
Copy link
Member

JounQin commented Mar 17, 2025

I'm not familiar with turborepo, contributing to nolyfill was a headache for a short time but good to try, maybe nx is also a promising solution, I didn't compare them in depth.

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

No branches or pull requests

3 participants