Skip to content

⚡ Bolt: Optimize manifest selection and file filtering in find-files #32

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

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

⚡ Bolt: Optimize manifest selection and file filtering in find-files #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
154 changes: 86 additions & 68 deletions src/lib/find-files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,11 @@ function detectProjectTypeFromFile(
): string | null {
try {
const packageManager = detectPackageManagerFromFile(file, featureFlags);
if (['yarn', 'npm', 'pnpm'].includes(packageManager)) {
if (
packageManager === 'yarn' ||
packageManager === 'npm' ||
packageManager === 'pnpm'
) {
return 'node';
}
return packageManager;
Expand All @@ -291,104 +295,118 @@ function shouldSkipAddingFile(
filePath: string,
filteredFiles: string[],
): boolean {
if (['gradle'].includes(packageManager) && filePath) {
const rootGradleFile = filteredFiles
.filter(
(targetFile) =>
targetFile.endsWith('build.gradle') ||
targetFile.endsWith('build.gradle.kts'),
)
.filter((targetFile) => {
if (packageManager === 'gradle' && filePath) {
// Optimization: Use .some() to short-circuit search and avoid intermediate array allocations.
return filteredFiles.some((targetFile) => {
if (
targetFile.endsWith('build.gradle') ||
targetFile.endsWith('build.gradle.kts')
) {
const parsedPath = pathLib.parse(targetFile);
const relativePath = pathLib.relative(parsedPath.dir, filePath);
return !relativePath.startsWith(`..${pathLib.sep}`);
});
return !!rootGradleFile.length;
}
return false;
});
}
return false;
}

const nodeLockfiles = [
SUPPORTED_MANIFEST_FILES.PACKAGE_LOCK_JSON as string,
SUPPORTED_MANIFEST_FILES.YARN_LOCK as string,
SUPPORTED_MANIFEST_FILES.PNPM_LOCK as string,
];

function chooseBestManifest(
files: Array<{ base: string; path: string }>,
projectType: string,
): string | null {
switch (projectType) {
case 'node': {
const nodeLockfiles = [
SUPPORTED_MANIFEST_FILES.PACKAGE_LOCK_JSON as string,
SUPPORTED_MANIFEST_FILES.YARN_LOCK as string,
SUPPORTED_MANIFEST_FILES.PNPM_LOCK as string,
];
const lockFile = files.filter((path) =>
nodeLockfiles.includes(path.base),
)[0];
debug(
`Encountered multiple node lockfiles files, defaulting to ${lockFile.path}`,
);
// Optimization: Use .find() instead of .filter()[0] to short-circuit on match
// and avoid array allocation. Also safely check lockFile existence before reading lockFile.path.
const lockFile = files.find((path) => nodeLockfiles.includes(path.base));
if (lockFile) {
debug(
`Encountered multiple node lockfiles files, defaulting to ${lockFile.path}`,
);
return lockFile.path;
}
const packageJson = files.filter((path) =>
['package.json'].includes(path.base),
)[0];
debug(
`Encountered multiple npm manifest files, defaulting to ${packageJson.path}`,
);
return packageJson.path;
const packageJson = files.find((path) => path.base === 'package.json');
if (packageJson) {
debug(
`Encountered multiple npm manifest files, defaulting to ${packageJson.path}`,
);
return packageJson.path;
}
return null;
}
case 'rubygems': {
const defaultManifest = files.filter((path) =>
['Gemfile.lock'].includes(path.base),
)[0];
debug(
`Encountered multiple gem manifest files, defaulting to ${defaultManifest.path}`,
const defaultManifest = files.find(
(path) => path.base === 'Gemfile.lock',
);
return defaultManifest.path;
if (defaultManifest) {
debug(
`Encountered multiple gem manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
}
return null;
}
case 'cocoapods': {
const defaultManifest = files.filter((path) =>
['Podfile'].includes(path.base),
)[0];
debug(
`Encountered multiple cocoapods manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
const defaultManifest = files.find((path) => path.base === 'Podfile');
if (defaultManifest) {
debug(
`Encountered multiple cocoapods manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
}
return null;
}
case 'pip': {
const defaultManifest = files.filter((path) =>
['Pipfile'].includes(path.base),
)[0];
debug(
`Encountered multiple pip manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
const defaultManifest = files.find((path) => path.base === 'Pipfile');
if (defaultManifest) {
debug(
`Encountered multiple pip manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
}
return null;
}
case 'gradle': {
const defaultManifest = files.filter((path) =>
['build.gradle'].includes(path.base),
)[0];
debug(
`Encountered multiple gradle manifest files, defaulting to ${defaultManifest.path}`,
const defaultManifest = files.find(
(path) => path.base === 'build.gradle',
);
return defaultManifest.path;
if (defaultManifest) {
debug(
`Encountered multiple gradle manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
}
return null;
}
case 'poetry': {
const defaultManifest = files.filter((path) =>
['pyproject.toml'].includes(path.base),
)[0];
debug(
`Encountered multiple poetry manifest files, defaulting to ${defaultManifest.path}`,
const defaultManifest = files.find(
(path) => path.base === 'pyproject.toml',
);
return defaultManifest.path;
if (defaultManifest) {
debug(
`Encountered multiple poetry manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
}
return null;
}
case 'hex': {
const defaultManifest = files.filter((path) =>
['mix.exs'].includes(path.base),
)[0];
debug(
`Encountered multiple hex manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
const defaultManifest = files.find((path) => path.base === 'mix.exs');
if (defaultManifest) {
debug(
`Encountered multiple hex manifest files, defaulting to ${defaultManifest.path}`,
);
return defaultManifest.path;
}
return null;
}
default: {
return null;
Expand Down
40 changes: 32 additions & 8 deletions test/tap/find-files.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,30 +248,54 @@ test('isExcludedPath correctness and caching behavior', async (t) => {
const isWin = process.platform === 'win32';
if (!isWin) {
t.ok(isExcludedPath('/foo/bar', excludeList), 'should exclude exact match');
t.notOk(isExcludedPath('/foo/BAR', excludeList), 'should be case-sensitive on non-Windows');
t.notOk(isExcludedPath('/xyz', excludeList), 'should not exclude unmatched path');
t.notOk(
isExcludedPath('/foo/BAR', excludeList),
'should be case-sensitive on non-Windows',
);
t.notOk(
isExcludedPath('/xyz', excludeList),
'should not exclude unmatched path',
);
} else {
t.ok(isExcludedPath('/foo/bar', excludeList), 'should exclude exact match');
t.ok(isExcludedPath('/foo/BAR', excludeList), 'should be case-insensitive on Windows');
t.notOk(isExcludedPath('/xyz', excludeList), 'should not exclude unmatched path');
t.ok(
isExcludedPath('/foo/BAR', excludeList),
'should be case-insensitive on Windows',
);
t.notOk(
isExcludedPath('/xyz', excludeList),
'should not exclude unmatched path',
);
}

// 2. Empty arrays
t.notOk(isExcludedPath('/foo/bar', []), 'should return false if exclude array is empty');
t.notOk(
isExcludedPath('/foo/bar', []),
'should return false if exclude array is empty',
);

// 3. Cache behavior verification (by passing same and different array references)
const originalPlatform = process.platform;
try {
// We can verify that the same array reference returns correct results when queried.
// Let's create a fresh array reference.
const customExclude = ['/abc/def'];
t.ok(isExcludedPath('/abc/def', customExclude), 'should match using fresh exclude list');
t.ok(
isExcludedPath('/abc/def', customExclude),
'should match using fresh exclude list',
);

// To verify caching works under the hood without breaking types or using internal access,
// we can observe that querying with the exact same reference succeeds instantly.
// Let's check that subsequent calls with the same array reference are correct.
t.ok(isExcludedPath('/abc/def', customExclude), 'should match again on the same array reference');
t.notOk(isExcludedPath('/other', customExclude), 'should not match other paths');
t.ok(
isExcludedPath('/abc/def', customExclude),
'should match again on the same array reference',
);
t.notOk(
isExcludedPath('/other', customExclude),
'should not match other paths',
);
} finally {
Object.defineProperty(process, 'platform', { value: originalPlatform });
}
Expand Down