feat!: move to ESM only #516
Conversation
|
One of the ci failures seems to be some funk with unsupported API imports I'll figure it out tomorrow |
lib/index.js
Outdated
There was a problem hiding this comment.
This is generated by hand now? Authors of new rules usually forget to update this kind of thing so just want to confirm tests/index.js verifies everything is included here properly?
There was a problem hiding this comment.
it is
we could write a script to auto generate it if that helps
|
For the node 21-23 test failures, I wonder if it's just the typechecking taking slightly longer in that first run of the rule. The earlier node versions that succeeded, the runtime of that rule was pretty long too, and somewhat close to the 2s timeout
|
|
yeah im pretty sure its just how long it takes typescript to run but im not sure what we can do about that other than increase the timeout for that specific test |
lib/rules/no-property-in-node.js
Outdated
There was a problem hiding this comment.
Thread about the test failure in Node 21-24.
Looks like it's just this rule only.
1068 passing (7s)
1 failing
1) no-property-in-node
valid
'a' in window;:
Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/eslint-plugin-eslint-plugin/eslint-plugin-eslint-plugin/tests/lib/rules/no-property-in-node.js)
at process.processImmediate (node:internal/timers:478:21)
CC: @JoshuaKGoldberg who authored the rule in case he has any ideas
There was a problem hiding this comment.
I'm updating TS dependencies just in case:
There was a problem hiding this comment.
Even if it's not something that gets merged, it'd be interesting to know if extending the timeout does indeed allow the tests to pass, or if it's something more significant
There was a problem hiding this comment.
I'll try and repro it locally, when i get home
There was a problem hiding this comment.
Running locally passes for me in half the timeout time (but still long). This is on node 22. So, that would suggest it's environment dependent and the type checking is running a bit longer on node 21-24.
There was a problem hiding this comment.
Increasing the mocha timeout from 2000ms to 3000ms (--timeout 3000) didn't fix it. Unclear how long the timeout would need to be to accommodate this test file.
There was a problem hiding this comment.
I could take a stab at moving the repo to vitest (in place of mocha / c8) in a separate change, if that would help.
There was a problem hiding this comment.
Yes, would be open to a separate PR to move to vitest first, assuming that has advantages over the current setup.
There was a problem hiding this comment.
@43081j this merged, if you want to rebase when you're able.
This change moves the test runner from `mocha` + `nyc` to `vitest`. In order to import `vitest` the tests had to be in esm, so I checked out everything under the `tests` folder from eslint-community#516 (leaving the source as it was). I also moved the `eslint-rule-tester` out of the `tests/lib` folder and into its own `utils` folder. That way vitest didn't treat it as a test. Note: rather than using `vitest`'s preferred `v8` coverage reporter, I used the `istanbul` one, to ensure the coverages between old and new were the same. I did notice that when I tried the `v8` coverage reporter, the coverage numbers were much less. Something to consider as a follow-up change. The v8 reporter *should* be more accurate, so the coverage may not be as high as it seems. I'd recommend moving up to the `v8` reporter after the esm branch lands.
* test: change test runner to vitest This change moves the test runner from `mocha` + `nyc` to `vitest`. In order to import `vitest` the tests had to be in esm, so I checked out everything under the `tests` folder from #516 (leaving the source as it was). I also moved the `eslint-rule-tester` out of the `tests/lib` folder and into its own `utils` folder. That way vitest didn't treat it as a test. Note: rather than using `vitest`'s preferred `v8` coverage reporter, I used the `istanbul` one, to ensure the coverages between old and new were the same. I did notice that when I tried the `v8` coverage reporter, the coverage numbers were much less. Something to consider as a follow-up change. The v8 reporter *should* be more accurate, so the coverage may not be as high as it seems. I'd recommend moving up to the `v8` reporter after the esm branch lands. * test: update fixtures tsconfig to use wildcard * build: remove `globals` and packageManager config
|
Nice work @43081j 🥳 |
Anyone who is using commonjs on node
<20will have to do one of the following:eslint.config.mjsEveryone else should be unaffected.
This would be a new major of course.
Let me know if there's any other changes you think we should do