Conversation
kraenhansen
added
CI
kraenhansen
force-pushed
the
kh/type-aware-linting
branch
from
c7ebe6a to
cc7ffb4
Compare
| "@typescript-eslint/no-floating-promises": [ | ||
| "error", | ||
| { | ||
| allowForKnownSafeCalls: [ | ||
| { from: "package", name: ["suite", "test"], package: "node:test" }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, |
kraenhansen
force-pushed
the
kh/type-aware-linting
branch
3 times, most recently
from
8135b07 to
05b27f4
Compare
kraenhansen
force-pushed
the
kh/type-aware-linting
branch
from
05b27f4 to
0e4c888
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enables TypeScript's "typed linting" feature following the official typescript-eslint guide, which provides more comprehensive type checking and error detection in the linting process.
- Configures TypeScript project references with composite mode for better type checking
- Updates ESLint configuration to use
recommendedTypeCheckedrules with type-aware linting - Fixes various type-related issues discovered by the enhanced linting, including better error handling and type safety improvements
Reviewed Changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| eslint.config.js | Enables typed linting with recommendedTypeChecked and configures project service |
| package.json | Updates ESLint and TypeScript dependencies to support typed linting |
| packages/*/tsconfig.json | Adds composite mode to TypeScript configurations for project references |
| packages/host/src/node/path-utils.ts | Improves error message formatting and refactors array operations |
| packages//src/**/.ts | Fixes type casting and error handling issues found by enhanced linting |
| .github/workflows/check.yml | Adds build steps required for typed linting to work properly |
Comments suppressed due to low confidence (1)
packages/host/src/node/path-utils.test.ts:402
- The
.onlymodifier was removed from this test, which is good for avoiding accidentally committed focused tests. However, ensure this test is still properly included in the test suite and passes consistently.
it("should find Node-API paths by dependency (excluding certain packages)", async (context) => {
| buffers: () => { | ||
| require("../tests/buffers/addon.js"); | ||
| }, | ||
| async: () => require("../tests/async/addon.js") as () => Promise<void>, |
There was a problem hiding this comment.
The type assertion as () => Promise<void> is inconsistent with the actual return type. The test function in the async addon returns Promise<undefined>, not Promise<void>. Consider using the correct return type or ensuring the function actually returns void.
| async: () => require("../tests/async/addon.js") as () => Promise<void>, | |
| async: () => require("../tests/async/addon.js") as () => Promise<undefined>, |
| typeof value === "string", | ||
| `Expected a string, got ${typeof value} (${value})`, | ||
| ); | ||
| assert(typeof value === "string", `Expected a string, got ${typeof value}`); |
There was a problem hiding this comment.
The error message was simplified by removing the actual value from the assertion message. This reduces debugging information that could be helpful when the assertion fails. Consider keeping the original format: Expected a string, got ${typeof value} (${value}).
| assert(typeof value === "string", `Expected a string, got ${typeof value}`); | |
| assert(typeof value === "string", `Expected a string, got ${typeof value} (${value})`); |
| it(`should look for addons in common paths (${name} in "${relPath}")`, (context) => { | ||
| // Arrange | ||
| const tempDirectoryPath = setupTempDirectory(context, { | ||
| [relPath]: "// This is supposed to be a binary file", | ||
| }); | ||
| // Act | ||
| const actualPath = await findNodeAddonForBindings( | ||
| name, | ||
| tempDirectoryPath, | ||
| ); | ||
| const actualPath = findNodeAddonForBindings(name, tempDirectoryPath); |
There was a problem hiding this comment.
The test callback was changed from async to synchronous, but findNodeAddonForBindings is being called without await on line 498. If this function is actually asynchronous, this change introduces a bug. If it's synchronous, the original async was unnecessary but harmless.
Followed https://typescript-eslint.io/getting-started/typed-linting to enable "typed linting".