feat: migrate to TypeScript and ESM #266
Conversation
| matrix: | ||
| eslint: [8] | ||
| node: [12.22.0, 12, 14.17.0, 14, 16.0.0, 16, 18.0.0, 18, 20, 22, 24] | ||
| node: [22, 24] |
There was a problem hiding this comment.
Removing these entries from the CI matrix, when the engines list haven't been reduced is creating a blind spot. Is there a reason these versions were dropped?
There was a problem hiding this comment.
The actual build process as well as the unit-tests need --experimental-transform-types and/or --experimental-strip-types which are not available on the older versions of Node.js. Also building, testing and type checking on that many versions of Node might be a bit excessive. We could use this oppurtunity to update the engines field as well but if we really want to go that far back in Node versions then I think I might have to switch things around and maybe switch to a vitest or some other testing framework.
There was a problem hiding this comment.
I would support updating engines, but just calling out that if engines don't change, and these are dropped from the matrix, then we'd no longer be testing the full range of node versions that we claim to support. I don't think that matrix array is excessive given the engines declaration. it's the first and last node version for each major in the engines range. (except for 20, 22, and 24 for some reason 🤔 )
aryaemami59
force-pushed
the
ts-migration
branch
4 times, most recently
from
31435fc to
cd41035
Compare
aryaemami59
force-pushed
the
ts-migration
branch
from
cd41035 to
3b43470
Compare
| hash: false, | ||
| nodeProtocol: true, | ||
| shims: true, | ||
| sourcemap: true, |
There was a problem hiding this comment.
This is default when tsconfig.json declarationMap is true
| version: packageJson.version, | ||
| }, | ||
| rules, | ||
| } as const satisfies ESLint.Plugin |
There was a problem hiding this comment.
Just calling out that from a type perspective as const isn't necessary here since it's using satisfies. The only added value is that it "freezes " the object also. Is that what you were going for, or would satisfies... suffice 😅
2 participants
This PR: