Skip to content

fix(scripts): make eslint run again on pre-commit #25537

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
Hotell merged 1 commit into from
Nov 10, 2022
Merged

fix(scripts): make eslint run again on pre-commit #25537

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
6 changes: 4 additions & 2 deletions scripts/lint-staged/eslint-for-package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ async function run() {
// (Note that until we start linting all files in the package and can remove the constants.directory
// segment here, the glob needs to start with the absolute package path in case someone has named
// the directory containing all their git repos "src".)
includePattern = path.join(packagePath, constants.directory, '**', `*{${constants.extensions}`);
includePattern = path.join(packagePath, constants.directory, '**', `*{${constants.extensions}}`);
eslint = new ESLint({ fix: true, cache: true });
} else {
// Otherwise, look for the --ext option to determine extensions
Expand All @@ -42,8 +42,10 @@ async function run() {
eslint = new ESLint({ fix: true, cache: lintScript.includes('--cache') });
}

// files are provided by @see `./eslint.js` via cli
const cliFiles = process.argv.slice(2);
// Filter out files with non-linted extensions
const files = process.argv.slice(2).filter(file => micromatch.isMatch(file, includePattern));
const files = cliFiles.filter(file => micromatch.isMatch(file, includePattern));

// Filter out ignored files (2-step process due to isPathIgnored returning a promise)
const ignoreResults = await Promise.all(files.map(f => eslint.isPathIgnored(f)));
Expand Down
8 changes: 7 additions & 1 deletion scripts/lint-staged/eslint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@ function groupFilesByPackage() {
);

for (const file of files) {
const packagePath = packagesWithEslint.find(packagePath => file.startsWith(packagePath));
Copy link
Contributor Author

@Hotell Hotell Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this wasn't working since convergence started! it was always resolving all staged files under v8 packages/react package 🚨

const packagePath = packagesWithEslint.find(packagePath => {
// if file lives within searched package we will get only shortened absolute path `/src/abc.ts`
// we add `.` to make it relative and thus have match pattern to check upon
const normalizedFilePath = file.replace(packagePath, '.');
Copy link
Contributor Author

@Hotell Hotell Nov 4, 2022

Choose a reason for hiding this comment

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

this is kinda naive but works. otherwise we would need to do real checks against files system which would be not the most performant solution.

If anyone has better idea how to handle please share !

return normalizedFilePath.startsWith('./');
});

// Exclude files in a package without an eslintrc (or not in a package at all)
if (packagePath) {
if (!filesByPackage[packagePath]) {
Expand Down