Skip to content

Conversation

@ZhangYiJiang
Copy link

@ZhangYiJiang ZhangYiJiang commented Jul 12, 2021

  • Add @types/node as a dependency - this is required for the NodeJS namespace to work (by the way an alternative is to publish the .d.ts file on https://github.com/DefinitelyTyped/DefinitelyTyped/ - this means TS users would have to import another package, but the upshot is that non-TS users won't have to transitively import the types)
  • Improve return types when splitMime is static - this is expected to be the case most of the time, so it seems useful to type it. It is a bit unwieldy, but the API is otherwise pretty simple and I think the improved return type is worth it
  • Fix export declaration - the module uses a CJS export, which is not default if esModuleInterop is not true in tsconfig, so the original typings would work in that case, but if it is unspecified the typings would actually be incorrect because it forces the caller to incorrectly use a default import
  • Add a TS test - similar to that used in DefinitelyTyped typings, useful for validating the updated typings work

- Add test.ts to validate typings
- Fix module export declaration
- Improve return value typing when splitMime is a static value
@hellovietduc
Copy link

I think the type of output should be stream.Transform instead of NodeJS.ReadableStream. This package uses buffer-peek-stream, which inherits from stream.Transform:

https://github.com/seangarner/node-buffer-peek-stream/blob/edc33ff015ef2ad51fdf5444ddc382cb4fd6f01a/buffer-peek-stream.js#L44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants