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

Run ESLint and Prettier linting in worker threads #548

Merged
merged 43 commits into from
Sep 24, 2021
Merged

Conversation

72636c
Copy link
Member

@72636c 72636c commented Sep 23, 2021

This gets us from

skuba process
ESLint process
Prettier process
tsc process

to

skuba process
├─ ESLint worker thread
╰─ Prettier worker thread
tsc process

Worker threads are stable in all Node.js versions we support: https://nodejs.org/docs/latest-v12.x/api/worker_threads.html#worker_threads_worker_threads


TODO:

  • Changeset
  • Integration tests
  • Concurrency limiter

This took an eternity because I went down a rabbit hole of parallelising
Prettier and had little to show for it based on very rudimentary
benchmarking of skuba itself; it seemed the overhead of spinning up
multiple worker processes was significant. I may give this another shot
with worker threads in future.

While this is undeniably more code that using the ESLint and Prettier
CLIs out of the box, it presents a few opportunities:

- Customising logging output, particularly for `--debug`ging.
- Taking the above point further: supporting Buildkite annotations,
  particularly when `lint`ing.
- A marginally faster execution and smaller resource footprint.
- Likelier compatibility with alternative module resolution approaches
  like Yarn PnP, which can make it harder to reliably locate other
  `.bin` tools when `exec`ing.
While this doesn't perform global linkage like `npm link`, its setup is
almost instantaneous compared to an arduous install process with the npm
command.
`fs-extra` is back, but only in `devDependencies`.
@72636c 72636c added the dino:snooze Snooze in Review Dino label Sep 23, 2021
@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2021

🦋 Changeset detected

Latest commit: 79a6b6e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@72636c 72636c removed the dino:snooze Snooze in Review Dino label Sep 23, 2021
@72636c

This comment has been minimized.

src/cli/lint/external.ts Outdated Show resolved Hide resolved
src/cli/lint.int.test.ts Outdated Show resolved Hide resolved
This doesn't seem to be necessary anymore.
Base automatically changed from format-in-process to master September 24, 2021 02:19
@72636c 72636c added the dino:snooze Snooze in Review Dino label Sep 24, 2021
@72636c 72636c marked this pull request as ready for review September 24, 2021 05:46
@72636c 72636c requested a review from a team as a code owner September 24, 2021 05:46
@72636c 72636c removed the dino:snooze Snooze in Review Dino label Sep 24, 2021
Copy link
Contributor

@etaoins etaoins left a comment

Choose a reason for hiding this comment

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

🏆

src/cli/lint/eslint.ts Outdated Show resolved Hide resolved
@72636c 72636c enabled auto-merge (squash) September 24, 2021 07:24
@72636c 72636c merged commit aee08f9 into master Sep 24, 2021
@72636c 72636c deleted the lint-in-process branch September 24, 2021 07:25
@github-actions github-actions bot mentioned this pull request Sep 24, 2021
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

Successfully merging this pull request may close these issues.

2 participants