Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve typings #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

None yet

2 participants