typescript: upgrade eslint to v9 - #1802
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "files.trimTrailingWhitespace": true, | ||
|
|
||
| "typescript.tsdk": "node_modules/typescript/lib", | ||
| "js/ts.tsdk.path": "node_modules/typescript/lib", |
There was a problem hiding this comment.
Is js/ts.tsdk.path a real setting? The documented key for pointing VS Code (and Cursor) at the workspace TypeScript is typescript.tsdk, which is what this was. If js/ts.tsdk.path is silently ignored, contributors lose "Use Workspace Version" and their editor falls back to the bundled TS — an easy source of phantom type errors. Was this an intentional rename, or a slip?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
I found three minor sites where I think we should actually fix the code rather than adding the suppression. The rest of it looks good to me.
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| } catch (error) { | ||
| console.log(`- missing ${filePath}`); |
There was a problem hiding this comment.
For what it is worth, this seems like a real bug to me. We don't know that fs.readFile threw because of a missing file; it could equally be permissions, etc. So I'd suggest actually printing the error in console.log, which would allow us to remove the no-unused-vars lint above.
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| await bag.readMessages( |
There was a problem hiding this comment.
Similarly, I think here we should consider fixing this to use for await (const result of bag.messageIterator({ topics: ... })) { ... } and getting rid of the lint annotation.
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| return reader.readMessage(data).toJSON(); |
There was a problem hiding this comment.
Similarly, I think we should change this to use toObject() instead of toJSON(), and then we can remove the no-deprecated lint option above.
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
Thanks for the fixes, looks good to me!
| } | ||
| const start = reader.offset; | ||
| const opcode = reader.uint8(); | ||
| const opcode: Opcode = reader.uint8(); |
There was a problem hiding this comment.
Does @mcap/core still typecheck after this? reader.uint8() returns number, and number isn't assignable to the numeric enum Opcode without a cast — so const opcode: Opcode = reader.uint8() should error TS2322. The old switch (opcode as Opcode) was carrying that cast, and dropping it moved the (now-invalid) coercion onto the annotation. If tsc is green, ignore me; otherwise keep const opcode = reader.uint8() with switch (opcode as Opcode), or write const opcode = reader.uint8() as Opcode.
2 participants
Changelog
None
Docs
None
Description
Upgrades ESLint, typescript-eslint,
@foxglove/eslint-pluginand remove some unneeded dependencies.I believe our eslint plugin is not yet ready for v10, but v9 is a significant change (requires rewriting config files) that will make a later v10 upgrade easier.