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

Make use of typescript import resolver #1509

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

Conversation

mrhyde
Copy link

@mrhyde mrhyde commented Apr 8, 2024

This pull request includes eslint-import-resolver-typescript as a peer dependency and enables TypeScript resolution for eslint-plugin-import. And sets import/no-unresolved to error

Doing so we will also avoid having extraneous looking dependency in consumers package.json:

...
    "eslint-config-love": "46.0.0",
    "eslint-import-resolver-typescript": "3.6.1",
...

@mrhyde
Copy link
Author

mrhyde commented Apr 30, 2024

Hi! Is there anything I can do to help get this feature merged and a new version published?

@mightyiam
Copy link
Owner

All efforts are currently towards flat config.

You could apply for joining the work sessions.

@mrhyde
Copy link
Author

mrhyde commented May 2, 2024

I couldn't find recent info on the sessions you mentioned. How do I apply?

@mightyiam
Copy link
Owner

It should be renamed but here it is:
https://mobusoperandi.com/mobs/standard.html

@mightyiam
Copy link
Owner

@mightyiam
Copy link
Owner

I'm sorry for breaking your PR, @mrhyde . I'd ask you to rebase, but I haven't yet acquired an understanding. Sorry for taking long.

@mrhyde
Copy link
Author

mrhyde commented May 27, 2024

Done :)

@mightyiam
Copy link
Owner

Thank you, @mrhyde.

I'm looking into this and after some understanding I find myself wanting to see a reproduction of the problem this solves.

Would you mind offering a branch where ESLint can't resolve an import and then another branch where that is solved using eslint-import-resolver-typescript, please?

@mrhyde
Copy link
Author

mrhyde commented May 31, 2024

There isn't a need to go to such lengths for something so simple. It essentially boils down to the fact that, without this plugin, ESLint cannot resolve TypeScript modules. Try enabling the import/no-unresolved rule, and you'll see for yourself.

@mightyiam
Copy link
Owner

Regarding this particular example; doesn't TypeScript make import/no-unresolved unnecessary?

@mrhyde
Copy link
Author

mrhyde commented May 31, 2024

It does, but it doesn't cover all scenarios. For example, when you need to run codemod or a simple find and replace that involves import statements. After that, if you run your linter on the staged files, everything will look okay.

@mightyiam
Copy link
Owner

Sorry, I lost you at typescript "doesn't cover all cases". What cases does it not cover, please? You mentioned codemod and find-replace but those are means of modification — they don't describe a case.

@mrhyde
Copy link
Author

mrhyde commented May 31, 2024

You don't want to use tsc as a linting tool because it's significantly slower, especially on large projects. Nor, adding a build step to your pre-commit hooks would negatively impact the development experience.

This PR provides a solution to have the module resolutions checked without actually running the compiler, thereby saving time.

@mightyiam
Copy link
Owner

May I know how long your compilation takes and how long your linting with this resolver takes, please?

@mrhyde
Copy link
Author

mrhyde commented Jun 1, 2024

It varies from project to project but approximately building is 2 to 4 times slower

@mightyiam
Copy link
Owner

I feel like I'm looking for absolute numbers.

When an import does not resolve we'd get both the linter and the compiler errors, correct?

@mightyiam
Copy link
Owner

In an editor, that is.

@mrhyde
Copy link
Author

mrhyde commented Jun 1, 2024

I feel like I'm looking for absolute numbers.

I feel like I need to clarify that it's not about the compiler time per se, but rather about the need to run a compiler just to get those errors. So it's not, for example, a comparison of 10 seconds of compiler time versus 6 seconds of linting time. Instead, it's about 6 seconds versus 16 seconds (compiler + linter) to get the same amount of information that the linter can provide on its own.

Yes, you can see both error messages in your IDE (VSCode in my case), but I have already mentioned scenarios where those messages might be missed.

@mrhyde
Copy link
Author

mrhyde commented Jun 2, 2024

❯ time npm run build
tsc --project tsconfig.build.json

user=14.81s system=0.11s cpu=118% total=12.559

❯ time npm run check:code
eslint . --ext .ts --report-unused-disable-directives --max-warnings 0

user=5.55s system=0.59s cpu=172% total=3.559

@mightyiam
Copy link
Owner

So, in your workflow, you found yourself using some tool to modify code. And you want to run ESLint to make sure that that tool did not mutilate import statements. But you're not concerned about other mutilations enough to run tsc, as well? In that case, is this a modification that acts solely on import statements? Does it merely reorganize your file structure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants