Conversation
|
@Marsup thoughts? |
kanongil
left a comment
kanongil
left a comment
There was a problem hiding this comment.
Here are some preliminary comments regarding the typescript integration. I haven't looked at the actual code conversion.
.github/workflows/ci-module.yml
Outdated
| uses: hapijs/.github/.github/workflows/ci-module.yml@master | ||
| with: | ||
| min-node-version: 14 | ||
| uses: hapijs/.github/.github/workflows/ci-module.yml@min-node-22-hapi-21 |
There was a problem hiding this comment.
I think we should still support node 20.
| "main": "lib/index.js", | ||
| "types": "lib/index.d.ts", | ||
| "main": "lib/index.ts", | ||
| "type": "module", |
There was a problem hiding this comment.
Switching to modules has implications for how this package can be consumed. It makes it more straightforward in common web contexts, but creates problems when used with commonjs due to async imports.
It also has implications for typescript integration, where local imports require different file extensions.
It might make sense to switch at this time, but it must be a deliberate decision.
There was a problem hiding this comment.
Hi, thanks for the reviews.
The move to "type": "module" is intentional.
package.json
Outdated
| "test-cov-html": "lab -a @hapi/code -r html -o coverage.html -L" | ||
| "test": "node --experimental-strip-types --test --experimental-test-coverage --test-coverage-branches=100 --test-coverage-functions=100 --test-coverage-lines=100 --test-coverage-exclude=test/** test/**/*.ts", | ||
| "lint": "tsc --noEmit && eslint . --ext .js,.ts", | ||
| "build": "tsc -p tsconfig.build.json --outDir dist" |
There was a problem hiding this comment.
A separate tsconfig for builds makes sense.
There was a problem hiding this comment.
What is the point of building into dist? The files are not even published, or otherwise exposed.
package.json
Outdated
| "repository": "git://github.com/hapijs/mimos", | ||
| "main": "lib/index.js", | ||
| "types": "lib/index.d.ts", | ||
| "main": "lib/index.ts", |
There was a problem hiding this comment.
I don't expect that exposing a .ts file is going to work for users. Node doesn't allow imported modules to be stripped: https://nodejs.org/docs/latest-v23.x/api/typescript.html#type-stripping-in-dependencies
| // constructor() | ||
| describe('Mimos', () => { | ||
|
|
||
| expect.type<Mimos>(mimos); |
There was a problem hiding this comment.
I'm sad to see that type testing has been removed.
jgarte
force-pushed
the
wip/ts
branch
3 times, most recently
from
6747391 to
2119429
Compare
|
Hi, I pushed some changes. Let me know what you think at this stage. Thanks for all the reviews. |
3 participants
Hi Hapi devs,
I did some work here in this branch on converting the
mimosrepository to TypeScript.Code review is much appreciated.
@damusix mentored me on this conversion.
all best.