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

Fix lint precommit issue #1555

Merged
merged 11 commits into from
Feb 7, 2025
Merged

Conversation

nickgros
Copy link
Collaborator

@nickgros nickgros commented Feb 6, 2025

Fix issue where vite config files were excluded from tsconfig, which would cause them to break on precommit when trying to lint them.

This issue still persists in synapse-react-client, where a handful of files are intentionally not included in the tsconfig because they have type errors

Fix issue where vite config files were excluded from tsconfig, which would cause them to break on precommit when trying to lint them.

This issue still persists in synapse-react-client, where a handful of files are intentionally not included in the tsconfig because they have type errors
@nickgros nickgros force-pushed the fix-eslint-precommit branch from 059073d to 9b4ab07 Compare February 7, 2025 14:24
Comment on lines 45 to 50
projectService: {
// We must enumerate each file that we want to lint that is not explicitly captured by a project-specific tsconfig
// allowDefaultProject does not allow `**` globs.
// https://github.com/typescript-eslint/typescript-eslint/issues/9739
allowDefaultProject: allProjectDirs.map(dir => `${dir}/*.ts`)
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes the issue -- any top-level .ts file will be linted against the top-level tsconfig.json

@@ -1,22 +1,25 @@
import { render, screen, waitFor, within } from '@testing-library/react'
Copy link
Collaborator Author

@nickgros nickgros Feb 7, 2025

Choose a reason for hiding this comment

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

Changed a bunch of tests and stories -- for all of them, I fixed type + lint errors and moved them inside of synapse-react-client/src.

Now synapse-react-client has no more non-linted/non type-checked tests + stories!

Comment on lines +12 to +27
const getDirectories = source =>
readdirSync(source, { withFileTypes: true })
.filter(dirent => dirent.isDirectory())
.map(dirent => dirent.name)

// TODO: Use Nx API to retrieve project directories
const appDirs = getDirectories(`${import.meta.dirname}/apps`).map(
app => `apps/${app}`,
)
const portalDirs = getDirectories(`${import.meta.dirname}/apps/portals`).map(
app => `apps/portals/${app}`,
)
const packageDirs = getDirectories(`${import.meta.dirname}/packages`).map(
pkg => `packages/${pkg}`,
)
const allProjectDirs = [...appDirs, ...portalDirs, ...packageDirs]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nx has a really friendly command-line API but an unintuitive JavaScript API for building things that are not plugins. Using Nx would be the right way to do this but I couldn't figure it out.

"baseUrl": "src",
"rootDir": "src",
"lib": ["dom", "dom.iterable", "esnext"],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update rootDir to src to match other projects

resetMocks: false,
},
],
reporters: [
'<rootDir>/test/testutils/LogForFailedTestsOnlyReporter.ts',
'<rootDir>/src/testutils/LogForFailedTestsOnlyReporter.cjs',
Copy link
Collaborator Author

@nickgros nickgros Feb 7, 2025

Choose a reason for hiding this comment

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

This was always a CommonJS file (it used require and had no type annotations). Jest has not been configured to transform this file, so the extension was updated to denote that it is in fact a CommonJS file.

@@ -0,0 +1,39 @@
import { render } from '@testing-library/react'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved file and fixed type/lint issues, test logic itself is not new

// allowDefaultProject does not allow `**` globs.
// https://github.com/typescript-eslint/typescript-eslint/issues/9739
allowDefaultProject: [
...allProjectDirs.map(dir => `${dir}/*.ts`),
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this entry?
...allProjectDirs.map(dir => ${dir}/*.tsx)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we have any top-level tsx files. So we shouldn't need it for now

Copy link
Member

@jay-hodgson jay-hodgson left a comment

Choose a reason for hiding this comment

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

Contingent on a successful build

@nickgros nickgros merged commit f91979b into Sage-Bionetworks:main Feb 7, 2025
24 checks passed
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