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

chore: migrate to mocha and eslint 9 #1266

Closed
wants to merge 1 commit into from
Closed

chore: migrate to mocha and eslint 9 #1266

wants to merge 1 commit into from

Conversation

43081j
Copy link
Collaborator

@43081j 43081j commented Sep 19, 2024

Migrates from Jest to mocha and node's own assert library.

Also updates ESLint to 9.x and bumps TSESLint to do the same.

@fb55 what do you think of this? jest is pretty hefty for what we need, this trims a fair chunk of the dependency weight out. mocha, tinyspy and node assert seem pretty clean together to me

similarly, I dropped most eslint customisation we had and just opted to rely on the recommended configs since prettier should pick up all the stylistic stuff we care about

Migrates from Jest to mocha and node's own `assert` library.

Also updates ESLint to 9.x and bumps TSESLint to do the same.
@@ -26,7 +26,7 @@
"parse5": "^7.0.0"
},
"scripts": {
"build:cjs": "tsc --module CommonJS --target ES6 --outDir dist/cjs && echo '{\"type\":\"commonjs\"}' > dist/cjs/package.json"
"build:cjs": "tsc --moduleResolution node10 --module CommonJS --target ES6 --outDir dist/cjs && echo '{\"type\":\"commonjs\"}' > dist/cjs/package.json"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typescript started to complain without this, since you must have module: "node16" if you have module resolution set to it (which we do)

I guess it got updated in the lockfile while updating tseslint etc and introduced the new behaviour

@fb55
Copy link
Collaborator

fb55 commented Sep 19, 2024

Nice!

Could we use Node's own test runner and skip having any dependencies?

@43081j
Copy link
Collaborator Author

43081j commented Sep 19, 2024

we could but I wasn't sure what node version matrix we wanted

the node test runner is available in 16.x onwards if I remember correctly

I'm also not sure off the top of my head how you get it to run typescript (via ts-node still presumably)

@43081j
Copy link
Collaborator Author

43081j commented Sep 19, 2024

maybe lets go with mocha for now and I can try the node runner in a separate PR? since I feel like it'll be a bit more work

@wooorm
Copy link
Collaborator

wooorm commented Sep 19, 2024

I’d love all this stuff, but separate PRs makes reviewing this easier!

@43081j
Copy link
Collaborator Author

43081j commented Sep 19, 2024

there's basically two changes (mocha and eslint). I could open two separate PRs but it gets a bit weird since I need to setup eslint to be happy with jest, knowing it'll go away immediately after

it may turn out its as easy as globals.jest but I'll have to try it out to find out

if you're unhappy with it going ahead without me splitting it up though I can have a look

@wooorm
Copy link
Collaborator

wooorm commented Sep 19, 2024

Right, sure. There are ways around that, with things like // todo(@43081j): remove this workaround when using builtin node test or so. I particularly said this because @fb55 suggested to use node:test instead of mocha anyway.

I’m not going to block this. But in my experience splitting PRs up into different things helps.

@43081j
Copy link
Collaborator Author

43081j commented Sep 19, 2024

yes, I think we should go ahead with mocha first though anyway since the node test runner will be more work

I'll see if I can split into two PRs

it wouldn't be a TODO for removing a workaround for node:test, it'd be temporary lint config to make ESLint happy about jest globals and what not. we'd then remove that in the mocha PR

@43081j
Copy link
Collaborator Author

43081j commented Sep 19, 2024

Closing in favour of #1268 and #1267

@43081j 43081j closed this Sep 19, 2024
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.

3 participants