Convert to TS#30
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial TypeScript type declarations for the package and wires typechecking into CI/tests so consumers get typed usage for the logger factory, Logger class, and the Hapi plugin export.
Changes:
- Added
index.tsdeclaration file describing the public API (hapiLogfactory,Logger, andplugin). - Added a TypeScript typecheck step (
tsc) and a TS “types test” that exercises the declared API. - Removed now-redundant JSDoc typedef blocks from runtime JS files and introduced a minimal
tsconfig.json.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Introduces TS configuration for strict, no-emit typechecking of the declarations and type tests. |
| index.ts | Defines the package’s TypeScript declarations for hapiLog, Logger, and plugin. |
| test/types.test.ts | Adds a compile-time-only usage test to validate the new typings. |
| package.json | Exposes the types entrypoint and adds typecheck to the test script; adds TS-related dev deps. |
| package-lock.json | Locks new dev dependencies added for typechecking. |
| lib/logger.js | Removes large JSDoc typedef/comment blocks now covered by the TS declarations. |
| index.js | Removes JSDoc for the exported factory function now covered by the TS declarations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "version": "11.0.0", | ||
| "description": "Hapi Log Plugin", | ||
| "main": "index.js", | ||
| "types": "index.ts", |
There was a problem hiding this comment.
package.json sets "types": "index.ts". In published packages, the types entry is expected to point to a declaration file (typically index.d.ts); pointing it at a .ts file can cause some consumers/tools to treat it as source and attempt to transpile or include it in builds. Consider renaming this file to index.d.ts (keeping the same export = style) and updating types accordingly.
| "types": "index.ts", | |
| "types": "index.d.ts", |
There was a problem hiding this comment.
Outdated advice. TS themselves recommend .ts over .d.ts.
martinj
left a comment
There was a problem hiding this comment.
Why not rewrite to ts entirely?
|
Also removed mocha/nyc/should in favor of native modules, and replaced eslint with biome. |
|
I've published this as a beta version, updated pa-services, deployed all four to dev and tested with my integration tests. |
No code changes, just adding TS types so I don't have to do
declare module '*'in my projects.