Skip to content

docs: add why section on the top to clarify #323

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 16, 2025
Merged

docs: add why section on the top to clarify #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
35a0087
docs: add `why` section on the top to clarify
JounQin May 14, 2025
0d2833a
docs: add the proof link of `heavy dependency on Typescript`
JounQin May 15, 2025
ae2c473
Apply suggestions from code review
JounQin May 15, 2025
2db56e5
Apply suggestions from code review
JounQin May 15, 2025
360a34b
chore: let's make AI happy
JounQin May 15, 2025
f74abd6
Apply suggestions from code review
JounQin May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .nano-staged.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export { default } from '@1stg/lint-staged/tsc'
export { default } from '@1stg/nano-staged/tsc'
39 changes: 39 additions & 0 deletions README.md
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I've been swamped this week but hoped to get to reviewing this before merge. Filing a bookmark to come back to you with a review soon. Sorry for the delay!

Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@

This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, and prevent issues with misspelling of file paths and import names. All the goodness that the ES2015+ static module syntax intends to provide, marked up in your editor.

It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster, through less [heavy dependency on Typescript](https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/src/exportMap/typescript.js#L16), and cleaner dependencies altogether.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to replace tsconfig-paths and heavy typescript under the hood, making it faster

  • How?
  • Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How?

replace tsconfig-paths and heavy typescript with single get-tsconfig?

Why?

typescript is heavy to load at https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/src/exportMap/typescript.js#L16


[`eslint-plugin-i` is now `eslint-plugin-import-x`](https://github.com/un-ts/eslint-plugin-import-x/issues/24#issuecomment-1991605123)

**IF YOU ARE USING THIS WITH SUBLIME**: see the [bottom section](#sublimelinter-eslint) for important info.

## TOC <!-- omit in toc -->

- [Why](#why)
- [Differences](#differences)
- [Installation](#installation)
- [Configuration (legacy: `.eslintrc*`)](#configuration-legacy-eslintrc)
- [TypeScript](#typescript)
Expand Down Expand Up @@ -50,6 +54,35 @@
- [Changelog](#changelog)
- [License](#license)

## Why

Many issues cannot be fixed easily without API changes. For example, see:

- <https://github.com/import-js/eslint-plugin-import/issues/1479>
- <https://github.com/import-js/eslint-plugin-import/issues/2108>
- <https://github.com/import-js/eslint-plugin-import/issues/2111>
Comment on lines +61 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(more on this on the next line) Having three issue links is a good start, but all three require a ton of reading. Two are labeled as help wanted. Someone cursorily reading these docs could easily come away with the conclusion that these issues are just pending in upstream, not actually blockers.


[`eslint-plugin-import`] refused to accept BREAKING CHANGES for these issues, so we had to fork it.
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence might be accurate, but it's not precise. Saying someone "refused" and you "had to fork" could mean a lot of things. As a result I think it sounds accusatory, even though that's not the intent. "refused" is a loaded word!

I think what this line is really trying to get at is why it is/was difficult to get these changes upstream. Right?

import-js/eslint-plugin-import#1479 (comment) & import-js/eslint-plugin-import#1479 (comment) -> the issue is marked as help wanted and there is no linked PR. So it's that it's just difficult & more work to satisfy the upstream's constraints, right?

import-js/eslint-plugin-import#2108 (comment) -> import-js/eslint-plugin-import#2108 (comment): same thing but with a draft PR that has a pending question. So it's similarly that it's just difficult & more work to work with their constraints, right?

import-js/eslint-plugin-import#2111: same thing with import-js/eslint-plugin-import#2111 (comment), just a 'no breaking changes' constraint that makes the design space harder?

Here's a first draft at a rewrite...

Suggested change
[`eslint-plugin-import`] refused to accept BREAKING CHANGES for these issues, so we had to fork it.
[`eslint-plugin-import`] prioritizes long backwards compatibility over than accepting breaking changes for these issues.
Efforts to resolve the issues in backwards-compatible ways upstreajm have stalled for several years.
This fork was created to have a plugin that does not have the same backwards compatibility requirements and so can more easily work for more modern codebases

...but I'm not confident that that rewrite is 100% right. I'm trying to convey exactly the technical reasons without implying any "blame". More word-crafting is probably required.

Copy link
Member Author

@JounQin JounQin May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import-js/eslint-plugin-import#2108 (comment) -> import-js/eslint-plugin-import#2108 (comment): same thing but with a draft PR that has a pending question. So it's similarly that it's just difficult & more work to work with their constraints, right?

I don't get what's blocking the PR actually. All tests are passing. And when the maintainer says "is this potentially a breaking change", I just don't want to continue that PR.

All possible breaking changes are refused. All the issues are in the same condition IMO.

cc @benmccann @43081j


[`eslint-plugin-import`] now claims in <https://github.com/un-ts/eslint-plugin-import-x/issues/170> that it will accept BREAKING CHANGES. However, still nothing is happening: <https://github.com/import-js/eslint-plugin-import/pull/3091>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked with ljharb? Is this just pending review? As an outsider, seeing a 7 month old semver-major PR without review makes me think there's some intrigue behind-the-scenes I'm not privy to.

Copy link
Member Author

@JounQin JounQin May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked with ljharb?

Why should I/we do that, we've already moved away for a long time?


[`eslint-plugin-import`] refuses to support the `exports` feature, and the maintainer even locked the feature request issue <https://github.com/import-js/eslint-plugin-import/issues/1810> to prevent future discussion. In the meantime, `eslint-plugin-import-x` now provides first-party support for the `exports` feature <https://github.com/un-ts/eslint-plugin-import-x/pull/209>, which will become the default in the next major version (v5).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refuses

It looks to me like the root issue of fixing resolve (browserify/resolve#224) is what was indicated as the desired fix, and that is similarly stalled due to difficulties in supporting the repo's long backwards compatibility requirements, yeah? So it's not that they're refusing, it's actively accepting - just difficult?

... to prevent future discussion

My read of import-js/eslint-plugin-import#1810 (comment) was that the lock was because one particular person was repeatedly arguming with the maintainer on that one day, and the conversation was getting heated & off-topic? The "for a bit" makes me think the conversation was supposed to be un-locked and that just never happened, unintentionally. Maybe now is the time for a ping to re-open?

Copy link
Member Author

@JounQin JounQin May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's not that they're refusing, it's actively accepting - just difficult?

Difficult due to backward compatibility, so it's just refusing.

My read of import-js/eslint-plugin-import#1810 (comment) was that the lock was because one particular person was repeatedly arguming with the maintainer on that one day, and the conversation was getting heated & off-topic? The "for a bit" makes me think the conversation was supposed to be un-locked and that just never happened, unintentionally. Maybe now is the time for a ping to re-open?

You may help to reconstruct the sentence, it's just our feelings, everyone could have different feelings.

Maybe now is the time for a ping to re-open?

Feel free to do that, while I'm not interested.


We haven't resolved all the issues yet, but we are working on them, which could happen in the next major version (v5): <https://github.com/un-ts/eslint-plugin-import-x/issues/235>.

## Differences

So what are the differences from `eslint-plugin-import` exactly?

- we target [Node `^18.18.0 || ^20.9.0 || >=21.1.0`](https://github.com/un-ts/eslint-plugin-import-x/blob/8b2d6d3b612eb57fb68c3fddec25b02fc622df7c/package.json#L12) + [ESLint `^8.57.0 || ^9.0.0`](https://github.com/un-ts/eslint-plugin-import-x/blob/8b2d6d3b612eb57fb68c3fddec25b02fc622df7c/package.json#L71), while `eslint-plugin-import` targets [Node `>=4`](https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/package.json#L6) and [ESLint `^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0 || ^8 || ^9`](https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/package.json#L115C16-L115C64)
- we don't depend on old and outdated dependencies, so [we have 49 dependencies](https://npmgraph.js.org/?q=eslint-plugin-import-x) compared to [117 dependencies for `eslint-plugin-import`](https://npmgraph.js.org/?q=eslint-plugin-import)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data request: the counts and links to npmgraph are a good start, but don't immediately describe what the real impact is. Dependency tree size is abstract and vague, and not an end-user metric (e.g. 1 of the 49 dependencies might be larger than all of the opposing 117 combined).

In order to get to any end-user-relevant info such as the actual unpacked module size you have to know to:

  1. Open both links
  2. Click on the root plugins in the visualizer
  3. Find each Module Size > Unpacked Size (module + dependencies)

I think most readers will not do that, so calling out the 6 MB vs 36 MB comparison here would be required for getting it known.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll get even less dependencies when dropping eslint-import-resolver-node: #272 (comment)

- `eslint-plugin-import` uses `tsconfig-paths` + `typescript` itself to load `tsconfig`s while we use the single `get-tsconfig` instead, which is much faster and cleaner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much faster

Prove it! Show me the data! 😄

As a developer, I have been trained to not believe any performance comparison not backed up by data. One should always back perf comparisons up with repeatable experiments.

Note that isolated experiments in get-tsconfig itself are not enough. The package might be fast in isolation but equal -or even slower!- in real-world usage. I need actual data from this specific plugin use case.

Copy link
Member Author

@JounQin JounQin May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prove it! Show me the data! 😄

https://github.com/privatenumber/get-tsconfig#features

I can't say for 3rd part libraries. And I'm not interested to prove it because it may be hard as you said.

Similar package: https://github.com/dominikg/tsconfck with benchmark https://github.com/dominikg/tsconfck/blob/main/docs/benchmark.md

And get-tsconfig starts because tsconfck doesn't have sync API: dominikg/tsconfck#31 (comment), see also dominikg/tsconfck#183

cc @privatenumber

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleaner

Why would an end-user care about this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we're cleaning the ecosystem? https://e18e.dev/guide/cleanup.html

- `eslint-plugin-import` uses [`resolve`] which doesn't support the `exports` field in `package.json` while we build our own rust-based resolver [`unrs-resolver`] instead, which is feature-rich and way more performant.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature-rich

This is abstract and non-tangible: what do you mean? How is this relevant to a reader?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to PR to replace them.

- Our [v3 resolver](./resolvers/README.md#v3) interface shares a single `resolver` instance by default which is used all across resolving chains so it would benefit from caching and memoization out-of-the-box
- ...

The list could be longer in the future, but we don't want to make it too long here. Hope you enjoy and let's get started.

## Installation

```sh
Expand Down Expand Up @@ -662,9 +695,15 @@
[MIT][] © [JounQin][]@[1stG.me][]

[`@typescript-eslint/parser`]: https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser
[`eslint-plugin-import`]: https://github.com/import-js/eslint-plugin-import
[`eslint-import-resolver-typescript`]: https://github.com/import-js/eslint-import-resolver-typescript
[`eslint_d`]: https://www.npmjs.com/package/eslint_d
[`eslint-loader`]: https://www.npmjs.com/package/eslint-loader
[`get-tsconfig`]: https://github.com/privatenumber/get-tsconfig
[`napi-rs`]: https://github.com/napi-rs/napi-rs

Check warning on line 703 in README.md

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest 

{"reason":"Unexpected unused definition, expected no definition or one or more references to ``napi-rs``","source":"remark-lint","ruleId":"no-unused-definitions","severity":1}

Check warning on line 703 in README.md

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest 

{"reason":"Unexpected unused definition, expected no definition or one or more references to ``napi-rs``","source":"remark-lint","ruleId":"no-unused-definitions","severity":1}

Check warning on line 703 in README.md

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest 

{"reason":"Unexpected unused definition, expected no definition or one or more references to ``napi-rs``","source":"remark-lint","ruleId":"no-unused-definitions","severity":1}

Check warning on line 703 in README.md

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest 

{"reason":"Unexpected unused definition, expected no definition or one or more references to ``napi-rs``","source":"remark-lint","ruleId":"no-unused-definitions","severity":1}

Check warning on line 703 in README.md

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest 

{"reason":"Unexpected unused definition, expected no definition or one or more references to ``napi-rs``","source":"remark-lint","ruleId":"no-unused-definitions","severity":1}

Check warning on line 703 in README.md

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest 

{"reason":"Unexpected unused definition, expected no definition or one or more references to ``napi-rs``","source":"remark-lint","ruleId":"no-unused-definitions","severity":1}
[`tsconfig-paths`]: https://github.com/dividab/tsconfig-paths
[`typescript`]: https://github.com/microsoft/TypeScript
[`unrs-resolver`]: https://github.com/unrs/unrs-resolver
[`resolve`]: https://www.npmjs.com/package/resolve
[`externals`]: https://webpack.github.io/docs/library-and-externals.html
[1stG.me]: https://www.1stG.me
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
},
"devDependencies": {
"@1stg/commitlint-config": "^5.0.6",
"@1stg/lint-staged": "^4.0.9",
"@1stg/nano-staged": "^0.1.1",
"@1stg/prettier-config": "^5.1.4",
"@1stg/remark-preset": "^3.1.1",
"@1stg/simple-git-hooks": "^2.0.1",
Expand Down
30 changes: 15 additions & 15 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,31 @@ __metadata:
languageName: node
linkType: hard

"@1stg/config@npm:^1.0.4, @1stg/config@npm:^1.0.5":
"@1stg/config@npm:^1.0.5":
version: 1.0.5
resolution: "@1stg/config@npm:1.0.5"
checksum: 10c0/f7e4d638f45ea458fc80763ad26402f4606fd2cf37edb282777ab804b1c8abea866185e8287328754675261dc0bcd741d55a88338844bbf79ddc476dbe9b9503
languageName: node
linkType: hard

"@1stg/lint-staged@npm:^4.0.9":
version: 4.0.9
resolution: "@1stg/lint-staged@npm:4.0.9"
"@1stg/nano-staged@npm:^0.1.1":
version: 0.1.1
resolution: "@1stg/nano-staged@npm:0.1.1"
dependencies:
"@1stg/config": "npm:^1.0.4"
"@1stg/prettier-config": "npm:^5.0.1"
"@1stg/tsconfig": "npm:^3.0.2"
"@1stg/config": "npm:^1.0.5"
"@1stg/prettier-config": "npm:^5.1.1"
"@1stg/tsconfig": "npm:^3.0.3"
"@pkgr/core": "npm:^0.2.2"
prettier: "npm:^3.5.3"
peerDependencies:
lint-staged: ">=12.1.0"
checksum: 10c0/bc576e1b9e8f0fe260f9b3d00b631baa3e74088c41f03936be7028e549fcc57f0210fc8bdd69e574935653e4315fbc91e3e579776ccd0af95448b7ab15780b87
nano-staged: ">=0.8.0"
checksum: 10c0/3e0652c36c06e5c14836bf2329eb0da9d8cf6e9bb1a59ef4bc27fb85672eea685936d1b9d997ec56c3673e5458ee8cfaf34a79be701798c29b48b542bb0b0dc0
languageName: node
linkType: hard

"@1stg/prettier-config@npm:^5.0.1, @1stg/prettier-config@npm:^5.1.4":
version: 5.1.4
resolution: "@1stg/prettier-config@npm:5.1.4"
"@1stg/prettier-config@npm:^5.1.1, @1stg/prettier-config@npm:^5.1.4":
version: 5.2.0
resolution: "@1stg/prettier-config@npm:5.2.0"
dependencies:
"@1stg/config": "npm:^1.0.5"
"@prettier/plugin-pug": "npm:^3.3.0"
Expand All @@ -57,7 +57,7 @@ __metadata:
prettier-plugin-toml: "npm:^2.0.4"
peerDependencies:
prettier: ^3.0.0
checksum: 10c0/7950dca3881be0604eff57191b34a0f7efc0377b756225ba7ec969c81ef5cf5db2183d574679c4a1d9224e5597e110d5e78f476aab768e15d434b1c5509bbc3b
checksum: 10c0/a9879a9a0a84aac41d6896009f109ca03f2544e836bc388df3c581a45c4b47e2369b2104eaf35f3bff9d22fef6f2c6bb0f9941f1381b89f5ad0388e805cec028
languageName: node
linkType: hard

Expand Down Expand Up @@ -94,7 +94,7 @@ __metadata:
languageName: node
linkType: hard

"@1stg/tsconfig@npm:^3.0.2, @1stg/tsconfig@npm:^3.0.3":
"@1stg/tsconfig@npm:^3.0.3":
version: 3.0.3
resolution: "@1stg/tsconfig@npm:3.0.3"
peerDependencies:
Expand Down Expand Up @@ -6346,7 +6346,7 @@ __metadata:
resolution: "eslint-plugin-import-x@workspace:."
dependencies:
"@1stg/commitlint-config": "npm:^5.0.6"
"@1stg/lint-staged": "npm:^4.0.9"
"@1stg/nano-staged": "npm:^0.1.1"
"@1stg/prettier-config": "npm:^5.1.4"
"@1stg/remark-preset": "npm:^3.1.1"
"@1stg/simple-git-hooks": "npm:^2.0.1"
Expand Down