vm: debug failing nightly browser test #1452
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
ryanio
force-pushed
the
debug-failing-nightly-karma
branch
from
0ae7189 to
161897e
Compare
|
okay the problem here is that karma was using the top level "peerDependencies": {
"typescript": "4.0.x || 4.1.x || 4.2.x || 4.3.x"
}I believe with npm v7 it installed these dependencies automatically (that's a bit tricky, and something to watch out for in the future). Our packages were still using TS v3.9 though because otherwise we would have run into the noImplicitAny error types solved in the following commit. In talking with Andrew about this, we have been patiently waiting to upgrade our TypeSciprt for access to named tuples to improve the client's global event bus typings. TypeScript doesn't follow semver so 3.9 -> 4.0 actually doesn't mean anything, so I think we are safe to upgrade across our monorepo. Just as a sanity check, after building fresh packages with the new typescript I ran ev's really nice interface diff checker script for every package that checks the local interface compared to latest on npm ( |
|
@ryanio thanks for the analysis, cool find! 😄 Since you guys have talked this through I would go along with the switch to TypeScript 4. What would be the final state here for this to be ready to be reviewed and merged? Would you remove the nightly test run triggers again and otherwise the PR stays the same? |
| @@ -913,7 +913,7 @@ export class BlockHeader { | |||
| try { | |||
| const header = (await blockchain.getBlock(hash)).header | |||
| return header | |||
| } catch (error) { | |||
| } catch (error: any) { | |||
There was a problem hiding this comment.
Where is this coming from? Is this a new linter rule? Or is it a TypeScript version related change?
There was a problem hiding this comment.
yes it was changed in v4, from my understanding it is a more strict enforcement of noImplicitAny (microsoft/TypeScript#36775 (comment))
There was a problem hiding this comment.
Seems to have been promoted by the Ethereum community. Really strange how small the world sometimes is! 😛
There was a problem hiding this comment.
(but makes sense, I like that)
thanks! great, yes just removed it. |
Fixes #1448
Note: This PR upgrades TypeScript to latest (
typescript: v4.4.2,ts-node: v10.2.1)