Switch from ts-node to tsx
#3188
Conversation
Codecov ReportAttention:
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
There are some files added to this PR, I think they should be |
There was a problem hiding this comment.
Should this be renamed from transition-child.cts to here? (Same with transition-cluster)
There was a problem hiding this comment.
I don't think it matters. I was just removing that as it's no longer imperative to have these file extensions with tsx. That said, it shouldn't have any impact on it running since tsx handles CJS and ESM just fine.
|
I checked out retesteth, I think for now we should skip with the original instructions (use ts-node) or we should figure out how to use |
There was a problem hiding this comment.
I checked out retesteth, I think for now we should skip with the original instructions (use ts-node) or we should figure out how to use
spawnwhich then spawns tsx processes 🤔
The transition cluster/child run just fine with tsx in my testing. If I do curlie -v POST 127.0.0.1:3000 when the transition cluster is running, I can see logs from the transition-child thread running if I add console logs so it's not an issue of the ethjs side of the setup running. It seems like the retesth instructions are out of date though as I'm not able to get it to initiate a test. I think I'm just not configuring my retesteth setup correctly.
packages/client/test/cli/cli.spec.ts
Outdated
| @@ -18,6 +18,7 @@ export function clientRunHelper( | |||
| return new Promise((resolve) => { | |||
| child.stdout.on('data', async (data) => { | |||
| const message: string = data.toString() | |||
| console.log(message) | |||
| @@ -62,7 +62,7 @@ | |||
| "@scure/base": "1.1.1", | |||
| "debug": "^4.3.3", | |||
| "ethereum-cryptography": "^2.1.2", | |||
| "lru-cache": "^7.18.3", | |||
| "lru-cache": "^10.0.0", | |||
There was a problem hiding this comment.
Without having checked if this is the case or not, it would be good if all out lru-cache deps would be Version aligned throughout the packages.
There was a problem hiding this comment.
10.0.0 is the correct version to match everything else in our usage so we should be good here. Some of our dev deps use older versions but that shouldn't impact us.
There was a problem hiding this comment.
Ah ok, but it (generally) wouldn't hurt to align the dev dep versions too here (also just a mention)
| @@ -45,7 +45,8 @@ import type { Common } from '@ethereumjs/common' | |||
| * --profile If this flag is passed, the state/blockchain tests will profile | |||
| */ | |||
|
|
|||
| const argv = minimist(process.argv.slice(2)) | |||
| //@ts-expect-error Typescript thinks there isn't a default export on minimist but there is | |||
| const argv = minimist.default(process.argv.slice(2)) | |||
There was a problem hiding this comment.
Side note: minimist is also causing problems with bun and we should consider to replace (should be simple).
There was a problem hiding this comment.
Okay, I think I might address this in a separate PR. Not sure how much of a rewrite that will be to switch to yargs.
There was a problem hiding this comment.
Yeah, sure, definitely nothing for this PR and generally just a mention.
|
Looks like everything is finally passing now so should be ready for review. |
| @@ -74,7 +70,7 @@ describe('[CLI]', () => { | |||
| // if http endpoint startup message detected, call http endpoint with RPC method | |||
| await wait(600) | |||
| const client = Client.http({ | |||
| port: 8562, | |||
| port: 8569, | |||
There was a problem hiding this comment.
Out of interest why does the port have to change?
There was a problem hiding this comment.
I think I had a port collision where multiple child processes (i.e individual tests) were started by vitest simultaneously and they both tried to grab the same port.
|
Cool, congrats! 🎉🙂 |
|
Ugh, just skimmed through the changes, wasn't aware that this has gone that extensive. 🤯 |
😬 It was mostly just find and replace (except for the |
Switches our monorepo's usage of
ts-nodeover totsxts-nodedep withtsxgloballyts-nodewithtsxtsxhappylru-cachedep indevp2pto get rid of= require('lru-cache')syntaxinstall-browser-depsnpm script to the monorepo root to install pinned versions of the browser testing dependencies (pinned to versions known to work with our code, should be updated if repo: update vitest #3191 is merged