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

feat(typescript-estree): switch to globby #2418

Merged
merged 1 commit into from Aug 24, 2020
Merged

feat(typescript-estree): switch to globby #2418

merged 1 commit into from Aug 24, 2020

Conversation

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Aug 23, 2020

Fixes #2398

If the user has a particularly large node_modules folder and uses globs for parserOption.project, then the glob library can spend a decent chunk of time searching the node_modules folder.
In some cases, this can be on the order of hundreds to thousands of milliseconds.
This wouldn't be a problem, but for safety and correctness during a persistent parse, we have to do this check for every call to the parser (i.e. every file that's being linted).
Over a whole project, this can easily add up to many, many seconds wasted.

Previously, we:

  • applied the project globs, one by one
  • then manually excluded tsconfigs from the list.

This meant that we are always slow. I remember I did this because I had issues getting glob's ignore option to work at all.

The solution

globby is a better glob library:

  • it accepts an array of globs, which saves us doing manual looping
  • it supports exclusion globs (globs prefixed with !), which are evaluated as part of the glob process
  • it has caching built in by default

This allows us to evaluate all of the project globs at once, as opposed to one at a time (so should be less duplicated work).
This also allows us to evaluate the projectFolderIgnoreList at the same time as the project globs (so should be no useless work done).

All of these together should cut the glob evaluation time down to ~50ms for the first parse, and ~2ms for each parse after that (due to caching).
For comparison, previously, in bad cases we would spend ~3-500ms, per project, per parsed file.

Example to illustrate how much faster this can potentially be:
For a project that provides 2 project globs and has 100 files.
Before: 300ms * 2 projects * 100 files = 60,000ms (60s)
After: 50ms + 2ms * 100 files = 250ms

This should also save a non-trival amount of time in other, more optimal setups.

BREAKING CHANGE:

  • removes the ability to supply a RegExp to projectFolderIgnoreList, and changes the meaning of the string value from a regex to a glob.
@bradzacher bradzacher added this to the 4.0.0 milestone Aug 23, 2020
@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented Aug 23, 2020

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@bradzacher bradzacher linked an issue that may be closed by this pull request Aug 23, 2020
@bradzacher bradzacher force-pushed the switch-to-globby branch from b9489ec to 2068462 Aug 24, 2020
Fixes #2398

If the user has a particularly large node_modules folder and uses globs for `parserOption.project`, then the `glob` library can spend a decent chunk of time searching the `node_modules` folder.
In some cases, this can be on the order of hundreds to thousands of milliseconds.
This wouldn't be a problem, but for safety and correctness during a persistent parse, we have to do this check for every call to the parser (i.e. every file that's being linted).
Over a whole project, this can easily add up to many, many seconds wasted.

Previously, we:
- applied the project globs, one by one
- then manually excluded `tsconfig`s from the list.

This meant that we are always slow. I remember I did this because I had issues getting `glob`'s `ignore` option to work at all.

## The solution

`globby` is a better glob library:
- it accepts an array of globs, which saves us doing manual looping
- it supports exclusion globs (globs prefixed with `!`), which  are evaluated as part of the glob process
- it has caching built in by default

This allows us to evaluate all of the `project` globs at once, as opposed to one at a time (so should be less duplicated work).
This also allows us to evaluate the `projectFolderIgnoreList` at the same time as the `project` globs (so should be no useless work done).

All of these together should cut the glob evaluation time down to ~50ms for the first parse, and ~2ms for each parse after that (due to caching).
For comparison, previously, in bad cases we would spend ~3-500ms, per project, per parsed file.

Example to illustrate how much faster this can potentially be:
For a project that provides 2 globs and has 100 files.
Before: 300ms * 2 * 100 = 60,000ms (60s)
After: 50ms + 2 * 100 = 250ms

This should also save a non-trival amount of time in other, more optimal setups.

BREAKING CHANGE:
- removes the ability to supply a `RegExp` to `projectFolderIgnoreList`, and changes the meaning of the string value from a regex to a glob.
@bradzacher bradzacher force-pushed the switch-to-globby branch from 2068462 to 6d9c28f Aug 24, 2020
@bradzacher bradzacher merged commit 789c439 into v4 Aug 24, 2020
11 checks passed
11 checks passed
Typecheck
Details
Unit tests Unit tests
Details
Code style and lint
Details
Run integration tests on primary Node.js version
Details
Run unit tests on other Node.js versions (10.x)
Details
Run unit tests on other Node.js versions (14.x)
Details
Publish the latest code as a canary version
Details
Publish the latest code as a v4 prerelease version
Details
Semantic Pull Request ready to be squashed
Details
codecov/patch Coverage not affected when comparing c4f67a2...6d9c28f
Details
codecov/project 92.94% (+0.00%) compared to c4f67a2
Details
@bradzacher bradzacher deleted the switch-to-globby branch Aug 24, 2020
bradzacher added a commit that referenced this pull request Aug 29, 2020
Fixes #2398

If the user has a particularly large node_modules folder and uses globs for `parserOption.project`, then the `glob` library can spend a decent chunk of time searching the `node_modules` folder.
In some cases, this can be on the order of hundreds to thousands of milliseconds.
This wouldn't be a problem, but for safety and correctness during a persistent parse, we have to do this check for every call to the parser (i.e. every file that's being linted).
Over a whole project, this can easily add up to many, many seconds wasted.

Previously, we:
- applied the project globs, one by one
- then manually excluded `tsconfig`s from the list.

This meant that we are always slow. I remember I did this because I had issues getting `glob`'s `ignore` option to work at all.

## The solution

`globby` is a better glob library:
- it accepts an array of globs, which saves us doing manual looping
- it supports exclusion globs (globs prefixed with `!`), which  are evaluated as part of the glob process
- it has caching built in by default

This allows us to evaluate all of the `project` globs at once, as opposed to one at a time (so should be less duplicated work).
This also allows us to evaluate the `projectFolderIgnoreList` at the same time as the `project` globs (so should be no useless work done).

All of these together should cut the glob evaluation time down to ~50ms for the first parse, and ~2ms for each parse after that (due to caching).
For comparison, previously, in bad cases we would spend ~3-500ms, per project, per parsed file.

Example to illustrate how much faster this can potentially be:
For a project that provides 2 globs and has 100 files.
Before: 300ms * 2 * 100 = 60,000ms (60s)
After: 50ms + 2 * 100 = 250ms

This should also save a non-trival amount of time in other, more optimal setups.

BREAKING CHANGE:
- removes the ability to supply a `RegExp` to `projectFolderIgnoreList`, and changes the meaning of the string value from a regex to a glob.
bradzacher added a commit that referenced this pull request Aug 29, 2020
Fixes #2398

If the user has a particularly large node_modules folder and uses globs for `parserOption.project`, then the `glob` library can spend a decent chunk of time searching the `node_modules` folder.
In some cases, this can be on the order of hundreds to thousands of milliseconds.
This wouldn't be a problem, but for safety and correctness during a persistent parse, we have to do this check for every call to the parser (i.e. every file that's being linted).
Over a whole project, this can easily add up to many, many seconds wasted.

Previously, we:
- applied the project globs, one by one
- then manually excluded `tsconfig`s from the list.

This meant that we are always slow. I remember I did this because I had issues getting `glob`'s `ignore` option to work at all.

## The solution

`globby` is a better glob library:
- it accepts an array of globs, which saves us doing manual looping
- it supports exclusion globs (globs prefixed with `!`), which  are evaluated as part of the glob process
- it has caching built in by default

This allows us to evaluate all of the `project` globs at once, as opposed to one at a time (so should be less duplicated work).
This also allows us to evaluate the `projectFolderIgnoreList` at the same time as the `project` globs (so should be no useless work done).

All of these together should cut the glob evaluation time down to ~50ms for the first parse, and ~2ms for each parse after that (due to caching).
For comparison, previously, in bad cases we would spend ~3-500ms, per project, per parsed file.

Example to illustrate how much faster this can potentially be:
For a project that provides 2 globs and has 100 files.
Before: 300ms * 2 * 100 = 60,000ms (60s)
After: 50ms + 2 * 100 = 250ms

This should also save a non-trival amount of time in other, more optimal setups.

BREAKING CHANGE:
- removes the ability to supply a `RegExp` to `projectFolderIgnoreList`, and changes the meaning of the string value from a regex to a glob.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

1 participant