Skip to content

feat(typescript-estree): replace globby w/ fast-glob #9518

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 4 commits into from
Aug 26, 2024
Merged

feat(typescript-estree): replace globby w/ fast-glob #9518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd91a00
refactor(espree): replace `globby` w/ `fast-glob`
SukkaW Jun 29, 2024
0f018eb
fix(estree): call fast glob properly
SukkaW Jul 10, 2024
15ac7be
chore(estree): add comments
SukkaW Jul 10, 2024
0dc3f09
Merge branch 'main' into replace-globby-with-fast-glob
SukkaW Aug 24, 2024
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 packages/typescript-estree/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"@typescript-eslint/types": "8.2.0",
"@typescript-eslint/visitor-keys": "8.2.0",
"debug": "^4.3.4",
"globby": "^11.1.0",
"fast-glob": "^3.3.2",
"is-glob": "^4.0.3",
"minimatch": "^9.0.4",
"semver": "^7.6.0",
Expand Down
43 changes: 25 additions & 18 deletions packages/typescript-estree/src/parseSettings/resolveProjectList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import debug from 'debug';
import { sync as globSync } from 'globby';
import { sync as globSync } from 'fast-glob';
import isGlob from 'is-glob';

import type { CanonicalPath } from '../create-program/shared';
Expand Down Expand Up @@ -56,15 +56,15 @@ export function resolveProjectList(

const projectFolderIgnoreList = (
options.projectFolderIgnoreList ?? ['**/node_modules/**']
)
.reduce<string[]>((acc, folder) => {
if (typeof folder === 'string') {
acc.push(folder);
}
return acc;
}, [])
// prefix with a ! for not match glob
.map(folder => (folder.startsWith('!') ? folder : `!${folder}`));
).reduce<string[]>((acc, folder) => {
if (typeof folder === 'string') {
acc.push(
// prefix with a ! for not match glob
folder.startsWith('!') ? folder : `!${folder}`,
);
}
return acc;
}, []);
Comment on lines +59 to +67
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bonus change, replace the .reduce.map chain with a single .reduce to avoid unnecessary iteration.


const cacheKey = getHash({
project: sanitizedProjects,
Expand Down Expand Up @@ -93,16 +93,23 @@ export function resolveProjectList(
const nonGlobProjects = sanitizedProjects.filter(project => !isGlob(project));
const globProjects = sanitizedProjects.filter(project => isGlob(project));

let globProjectPaths: string[] = [];

if (globProjects.length > 0) {
// Although fast-glob supports multiple patterns, fast-glob returns arbitrary order of results
// to improve performance. To ensure the order is correct, we need to call fast-glob for each pattern
Copy link

Choose a reason for hiding this comment

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

OOC, does the order matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a unit test case against the order, so yes, it does matter.

Copy link

Choose a reason for hiding this comment

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

But why does the order matter? Shouldn't the unit test just check that the needed files are linted?

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Folks sometimes do things like ['packages/*/tsconfig.json', 'tsconfig.json'] - and/or entries like 'tsconfig.eslint.json'. The order intent is to first try the lower-level TSConfigs first.

Copy link

Choose a reason for hiding this comment

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

Gotcha, thanks!

// separately and then concatenate the results in patterns' order.
globProjectPaths = globProjects.flatMap(pattern =>
globSync(pattern, {
cwd: options.tsconfigRootDir,
ignore: projectFolderIgnoreList,
}),
);
}

const uniqueCanonicalProjectPaths = new Map(
nonGlobProjects
.concat(
globProjects.length === 0
? []
: globSync([...globProjects, ...projectFolderIgnoreList], {
cwd: options.tsconfigRootDir,
dot: true,
}),
)
.concat(globProjectPaths)
.map(project => [
getCanonicalFileName(
ensureAbsolutePath(project, options.tsconfigRootDir),
Expand Down
48 changes: 29 additions & 19 deletions packages/typescript-estree/tests/lib/parse.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { join, resolve } from 'node:path';

import type { CacheDurationSeconds } from '@typescript-eslint/types';
import debug from 'debug';
import * as globbyModule from 'globby';
import * as fastGlobModule from 'fast-glob';
import type * as typescriptModule from 'typescript';

import * as parser from '../../src';
Expand Down Expand Up @@ -39,11 +39,11 @@ jest.mock('typescript', () => {
};
});

jest.mock('globby', () => {
const globby = jest.requireActual<typeof globbyModule>('globby');
jest.mock('fast-glob', () => {
const fastGlob = jest.requireActual<typeof fastGlobModule>('fast-glob');
return {
...globby,
sync: jest.fn(globby.sync),
...fastGlob,
sync: jest.fn(fastGlob.sync),
};
});

Expand All @@ -52,7 +52,7 @@ const hrtimeSpy = jest.spyOn(process, 'hrtime');
const createDefaultCompilerOptionsFromExtra = jest.mocked(
sharedParserUtilsModule.createDefaultCompilerOptionsFromExtra,
);
const globbySyncMock = jest.mocked(globbyModule.sync);
const fastGlobSyncMock = jest.mocked(fastGlobModule.sync);

/**
* Aligns paths between environments, node for windows uses `\`, for linux and mac uses `/`
Expand Down Expand Up @@ -685,6 +685,12 @@ describe('parseAndGenerateServices', () => {

describe('cacheLifetime', () => {
describe('glob', () => {
const project = ['./**/tsconfig.json', './**/tsconfig.extra.json'];
// fast-glob returns arbitrary order of results to improve performance.
// `resolveProjectList()` calls fast-glob for each pattern to ensure the
// order is correct.
// Thus the expected call time of spy is the number of patterns.
const expectFastGlobCalls = project.length;
function doParse(lifetime: CacheDurationSeconds): void {
parser.parseAndGenerateServices('const x = 1', {
cacheLifetime: {
Expand All @@ -693,52 +699,56 @@ describe('parseAndGenerateServices', () => {
disallowAutomaticSingleRunInference: true,
filePath: join(FIXTURES_DIR, 'file.ts'),
tsconfigRootDir: FIXTURES_DIR,
project: ['./**/tsconfig.json', './**/tsconfig.extra.json'],
project,
});
}

it('should cache globs if the lifetime is non-zero', () => {
doParse(30);
expect(globbySyncMock).toHaveBeenCalledTimes(1);
expect(fastGlobSyncMock).toHaveBeenCalledTimes(expectFastGlobCalls);
doParse(30);
// shouldn't call globby again due to the caching
expect(globbySyncMock).toHaveBeenCalledTimes(1);
// shouldn't call fast-glob again due to the caching
expect(fastGlobSyncMock).toHaveBeenCalledTimes(expectFastGlobCalls);
});

it('should not cache globs if the lifetime is zero', () => {
doParse(0);
expect(globbySyncMock).toHaveBeenCalledTimes(1);
expect(fastGlobSyncMock).toHaveBeenCalledTimes(expectFastGlobCalls);
doParse(0);
// should call globby again because we specified immediate cache expiry
expect(globbySyncMock).toHaveBeenCalledTimes(2);
// should call fast-glob again because we specified immediate cache expiry
expect(fastGlobSyncMock).toHaveBeenCalledTimes(
expectFastGlobCalls * 2,
);
});

it('should evict the cache if the entry expires', () => {
hrtimeSpy.mockReturnValueOnce([1, 0]);

doParse(30);
expect(globbySyncMock).toHaveBeenCalledTimes(1);
expect(fastGlobSyncMock).toHaveBeenCalledTimes(expectFastGlobCalls);

// wow so much time has passed
hrtimeSpy.mockReturnValueOnce([Number.MAX_VALUE, 0]);

doParse(30);
// shouldn't call globby again due to the caching
expect(globbySyncMock).toHaveBeenCalledTimes(2);
// shouldn't call fast-glob again due to the caching
expect(fastGlobSyncMock).toHaveBeenCalledTimes(
expectFastGlobCalls * 2,
);
});

it('should infinitely cache if passed Infinity', () => {
hrtimeSpy.mockReturnValueOnce([1, 0]);

doParse('Infinity');
expect(globbySyncMock).toHaveBeenCalledTimes(1);
expect(fastGlobSyncMock).toHaveBeenCalledTimes(expectFastGlobCalls);

// wow so much time has passed
hrtimeSpy.mockReturnValueOnce([Number.MAX_VALUE, 0]);

doParse('Infinity');
// shouldn't call globby again due to the caching
expect(globbySyncMock).toHaveBeenCalledTimes(1);
// shouldn't call fast-glob again due to the caching
expect(fastGlobSyncMock).toHaveBeenCalledTimes(expectFastGlobCalls);
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5915,8 +5915,8 @@ __metadata:
"@typescript-eslint/types": 8.2.0
"@typescript-eslint/visitor-keys": 8.2.0
debug: ^4.3.4
fast-glob: ^3.3.2
glob: "*"
globby: ^11.1.0
is-glob: ^4.0.3
jest: 29.7.0
minimatch: ^9.0.4
Expand Down
Loading