Fixed package import issues#12
Conversation
- Enabled DTS generation by fixing type errors - Fixed `main` reference in package.json - Use type imports in generated models Also, updated jsdoc references
There was a problem hiding this comment.
Pull Request Overview
This PR fixes package import issues for the @docker/node-sdk library by enabling TypeScript declaration file generation and resolving import-related problems.
- Enabled DTS generation by fixing type errors and switching
dts: falsetodts: true - Fixed the
mainentry point in package.json to use the correct ESM extension - Updated all imports in generated models to use type-only imports for better performance
Reviewed Changes
Copilot reviewed 107 out of 109 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tsup.config.ts | Enabled DTS generation by changing dts from false to true |
| package.json | Fixed main entry point and added missing type dependencies |
| templates/model/model.mustache | Updated template to generate type-only imports |
| lib/util.ts | Added new utility functions for error handling and parsing |
| lib/http.ts | Improved error handling and type safety |
| lib/docker-client.ts | Enhanced error handling and refactored utility usage |
| lib/ssh.ts | Fixed SSH method name from forwardInStreamLocal to forwardOutStreamLocal |
| lib/socket.ts | Added proper type imports and improved callback signature |
| lib/tls.ts | Updated to use new utility function for error message extraction |
| lib/models/*.ts | Updated all model files to use type-only imports |
| test/util.test.ts | Added comprehensive test coverage for utility functions |
| README.md | Added installation instructions and import example |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| sshStream.emit('connect'); | ||
| }); | ||
| conn.openssh_forwardOutStreamLocal( |
There was a problem hiding this comment.
The method name appears to be incorrect. Based on the context of creating a Unix socket connection through SSH, this should likely be openssh_forwardInStreamLocal rather than openssh_forwardOutStreamLocal. The 'In' variant is typically used for incoming connections to local sockets.
| conn.openssh_forwardOutStreamLocal( | |
| conn.openssh_forwardInStreamLocal( |
There was a problem hiding this comment.
@ndeloof heads up: i switched to openssh_forwardOutStreamLocal b/c conn.openssh_forwardInStreamLocal had type errors b/c the callback does not have a stream parameter. openssh_forwardOutStreamLocal does pass stream in the callback.
| const onError = (error: Error) => { | ||
| socket.removeListener('connect', onConnect); | ||
| callback(error); | ||
| callback(error, socket); |
There was a problem hiding this comment.
The callback is being called with both an error and a socket parameter even when there's an error. According to Node.js callback conventions, when an error occurs, the second parameter should typically be undefined or omitted. Consider calling callback(error) instead.
| callback(error, socket); | |
| callback(error); |
| @@ -177,13 +177,12 @@ export class DockerClient { | |||
| } | |||
| } catch (parseError) { | |||
| // Skip invalid meta.json files | |||
There was a problem hiding this comment.
The continue statement was removed but the comment suggests skipping invalid files. Either the continue statement should be restored, or the comment should be updated to reflect the new behavior of not skipping invalid files.
| // Skip invalid meta.json files | |
| // Ignore invalid meta.json files |
- Configured separate projects for ESM (`esm-project`) and CJS (`cjs-project`) compatibility testing. - Updated CI workflow to include integration tests for both formats. - Added `.gitignore` files to exclude `node_modules` and lockfiles in test projects. - Verified `DockerClient` imports and functionality for both module types.
What I did
Fixed issues importing
from '@docker/node-sdk'How I did it
mainreference in package.jsonHow to verify it
I created a playground project to import
@docker/node-sdkand get running containers.Human readable description for the release notes