Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Performance - Linting with Type Information #104

Closed
BarryThePenguin opened this issue Oct 29, 2020 · 8 comments · Fixed by #114
Closed

Performance - Linting with Type Information #104

BarryThePenguin opened this issue Oct 29, 2020 · 8 comments · Fixed by #114

Comments

@BarryThePenguin
Copy link
Contributor

From https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/TYPED_LINTING.md

...by including parserOptions.project in your config, you are essentially asking TypeScript to do a build of your project before ESLint can do its linting. For small projects this takes a negligible amount of time (a few seconds); for large projects, it can take longer (30s or more).

Most of our users are fine with this, as they think the power of type-aware static analysis is worth it. Additionally, most users primarily consume lint errors via IDE plugins which, through some caching magic, do not suffer the same penalties. This means that generally they usually only run a complete lint before a push, or via their CI, where the extra time really doesn't matter.

Using eslint-config-xo-typescript with atom-linter-xo, Atom can hang and take upwards of 30 seconds to process a file when opening it.

It's pointed out by the linter-ui busy signal indicator History, which mentions "XO: running on foo.ts (29s)"

I've opened this issue here as my initial thought was to have a way of disabling typed linting while using an editor. After reading the above link, a better solution might be to figure out what is causing Atom to hand while atom-linter-xo processes the file?

@sindresorhus
Copy link
Owner

a better solution might be to figure out what is causing Atom to hand while atom-linter-xo processes the file?

Definitely this. It should not block at all.

@sindresorhus sindresorhus transferred this issue from xojs/eslint-config-xo-typescript Nov 2, 2020
@BarryThePenguin
Copy link
Contributor Author

BarryThePenguin commented May 12, 2021

To fix this issue, and #108, it looks like atom-linter-xo would need to adopt a Task based approach. This is just based on me investigating how linter-eslint does it

I'd be interested in taking this on. My initial thoughts are..

  • I'm unsure if having the Task handle multiple lint/fix jobs at the same time is necessary?
  • If multiple concurrent jobs is useful, should multiple Task instances be used to manage the jobs, or should a single Task instance run all jobs in additional child processes? The second option is similar to how linter-eslint approaches it.
  • Are there any libraries that might help simplify any of this work? From what I understand child_process in electron is a little different to what's in node, so there might be compatibility issues there..

Am I overthinking this at all? 😅

In addition to this work, would you be open to PRs for updating dependencies, and maybe revisit TypeScript?

@sindresorhus
Copy link
Owner

I'm unsure if having the Task handle multiple lint/fix jobs at the same time is necessary?

I think it could improve performance, but I'm not sure it's worth the added complexity and also it has to be limited to maybe 4-8 tasks. I think ESLint is planning on adding concurrent listing, so it might be worth just waiting on that.

@sindresorhus
Copy link
Owner

I think we should just try using one Task for now and see how it goes.

@sindresorhus
Copy link
Owner

would you be open to PRs for updating dependencies, and maybe revisit TypeScript?

Sure. Feel free to migrate to TypeScript if that would help you.

@BarryThePenguin
Copy link
Contributor Author

I think ESLint is planning on adding concurrent listing, so it might be worth just waiting on that.

Ok, single Task running individual jobs. Then when ESLint supports concurrency we can just take advantage of that 👍🏻

Sure. Feel free to migrate to TypeScript if that would help you.

I forgot I had already created #102 before leaving that comment, sorry! I think introducing the Task is enough new complexity..

@BarryThePenguin
Copy link
Contributor Author

I've got stuck trying to figure out how to handle the module syntax correctly.. There's a few pieces..

  1. Currently there is some usage of a /** @babel */ comment, which atom uses to automatically transpile with an old version of Babel (v5, maybe v6?)
  2. Some packages are also published as native ESM. So I can either use a previous version that still supports commonjs, or transpile those packages

To transpile native ESM packages, I'd probably need use either https://github.com/atom/atom-babel7-transpiler or https://github.com/smhxx/atom-ts-transpiler to handle any native ESM packages.

@sindresorhus
Copy link
Owner

So I can either use a previous version that still supports commonjs, or transpile those packages

That would probably be the simplest solution for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants