chore: migrate to esm#465
Conversation
|
Note: still need to test end to end to make sure everything is running correctly especially the web hooks |
wrslatz
left a comment
wrslatz
left a comment
There was a problem hiding this comment.
I know it's still in draft, but I left a few suggestions based on my understanding of migrating to ESM and some quick spot checking against resources and searching.
Since Private Mirrors only supports Node.js 22 and above, I believe the project could interop with ESM and CJS dependencies where it could not before, so the same ESM errors may not be present as before. Also, since the project is not published as a JS package, pure ESM has less impact to downstream consumers. I still think it's worth while to adopt.
| "type-check": "tsc --noEmit", | ||
| "bot:build": "tsc ./src/bot/index.ts --noEmit false --esModuleInterop --outDir ./build", | ||
| "bot:start": "probot run ./build/index.js", | ||
| "bot:start": "tsx ./src/bot/main.ts", |
There was a problem hiding this comment.
If we are using a new enough Node.js, we may be able to run directly using node since type stripping for TypeScript is now supported.
There was a problem hiding this comment.
Agreed on type stripping but I'm not sure that we need these scripts at all. I suppose you could use this to run the webhook handling logic in conjunction with the webhook relay but the dev script already includes this alongside the rest of the app and is set up to run the relay with concurrently.
| "format": "prettier --check .", | ||
| "format:fix": "prettier --write .", | ||
| "type-check": "tsc --noEmit", | ||
| "bot:build": "tsc ./src/bot/index.ts --noEmit false --esModuleInterop --outDir ./build", |
There was a problem hiding this comment.
For building, we could likely continue to use tsc or potentially use something like esbuild
| "target": "es5" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */, | ||
| "module": "commonjs" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */, | ||
| "target": "ES2022" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */, | ||
| "module": "preserve" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */, |
There was a problem hiding this comment.
Should be ESNext or NodeNext, I believe
| "noFallthroughCasesInSwitch": true /* Report errors for fallthrough cases in switch statement. */, | ||
| /* Module Resolution Options */ | ||
| "moduleResolution": "node" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */, | ||
| "moduleResolution": "bundler" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */, |
There was a problem hiding this comment.
Should be NodeNext I believe
There was a problem hiding this comment.
Should should be .mjs suffixed, I believe
There was a problem hiding this comment.
Ideally I preferprettier.config.mjs. This page has the different options in the order they are referenced but I like that format since it matches eslint's config file. You can also add // @ts-check to the top of the file so it's treated the same as the rest of our typescript code. While you can use the .ts extension there is some trickiness there mentioned in the eslint docs.
|
Renovate's tsconfig may be a good one to compare against, for this change and for other settings: https://github.com/renovatebot/renovate/blob/main/tsconfig.json |
I'm partial to the tsconfig bases that you pointed me at at one point. They have one for Next.js which should hopefully simplify the tsconfig a lot. On a typical project we would want to extend node24 and node-ts configs but I'm not sure how they might cross over with the Next one. I think node-ts should probably still be used (or referenced for additions) to ensure that we can use |
Good call. We should be able to merge together a couple of these at least and get the recommended settings. |
3 participants
Pull Request
Proposed Changes
Closes #331
Readiness Checklist
Author/Contributor
npm run formatand fix any formatting issues that have been introducednpm run lintand fix any linting issues that have been introducednpm run testand run tests