Allow file-type options to be directly passed to exported functions #752
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables the exported file-type functions to accept a new options parameter, making it easier to pass custom settings and enhancing test coverage.
- File-type functions now accept an options parameter to customize behavior.
- Documentation was updated to centralize file-type options and improve clarity.
- Test coverage was enhanced to cover the new options parameter in various functions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| readme.md | Updated documentation to include options parameters and anchors. |
| index.test-d.ts | Added tests to verify behavior when options are passed. |
| index.js | Modified exported functions to accept and forward options. |
| core.js | Updated internal functions to propagate new options parameter. |
218fe7e to
66dca8c
Compare
Improve coverage of type tests Centralized file-type options documentation
Improved documentation consistency for `customDetectors` in `options`. Reversed using FileTypeParser constructor, in "Example adding a detector", as I believe it adds value showing that. As, that way you can reuse the tweaked parser.
66dca8c to
9f87f29
Compare
6b121df to
6264f68
Compare
index.test-d.ts
Outdated
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromFile('myFile')); | ||
|
|
||
| // From a Node Readable (stream) | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromStream(new Readable())); | ||
|
|
||
| // From a DOM type Blob | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromBlob(new Blob())); | ||
|
|
||
| // From a DOM type Web byte ReadableStream | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromStream(new ReadableStream({type: 'bytes'}))); | ||
|
|
||
| // From a Node type byte ReadableStream | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromStream(new NodeReadableStream({type: 'bytes'}))); |
There was a problem hiding this comment.
All these tests are moot as they don't do anything other than just duplicating what the type signature already says. Type tests are only useful when they test something more advanced than just confirming what the actual types say.
There was a problem hiding this comment.
Certainly not moot.
Some of the types are not that straightforward, for example the ReadableStream which is both defined on the global DOM space, as well in Node.js, as a different type. The implementation is designed to accept either of those. I don't see any issue with testing / confirming that and to ensure that and that this behavior is kept in tact. In addition to that, is also helps to clarify the design. This also helps to get overview the high level API definition are typed in a consistent way.
There was a problem hiding this comment.
Type tests can be useful for advanced types, but just testing what it accepts is simply duplicating the type definition. index.test-d.ts was a mistake in general. Most of the time, just getting TS to validate the index.d.ts is enough.
There was a problem hiding this comment.
I tried to make it more effective, and got across an issue of using mixed options (FileTypeOptions and StreamOptions), lacking right declaration. Something I could have found as well by having a critical look to the TypeScript declaration, indeed.
Corrected that, as well I (believe) improved declaration of allowing mixed 2f6aad8, apparently an intersection works better. From a mathematical definition this does not make sense to me, but in the TypeScript world this seems to better reflect the type intended.
If there are things in ndex.test-d.ts, which you believe are moot, please just toss them out. For me it to arbitrary what should be in or out. I believe it is now in a more sensible and consistent state, as when found it prior to this PR.
index.test-d.ts
Outdated
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromFile('myFile')); | ||
|
|
||
| // From a Node Readable (stream) | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromStream(new Readable())); | ||
|
|
||
| // From a DOM type Blob | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromBlob(new Blob())); | ||
|
|
||
| // From a DOM type Web byte ReadableStream | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromStream(new ReadableStream({type: 'bytes'}))); | ||
|
|
||
| // From a Node type byte ReadableStream | ||
| expectType<FileTypeResult | undefined>(await fileTypeParser.fromStream(new NodeReadableStream({type: 'bytes'}))); |
There was a problem hiding this comment.
Type tests can be useful for advanced types, but just testing what it accepts is simply duplicating the type definition. index.test-d.ts was a mistake in general. Most of the time, just getting TS to validate the index.d.ts is enough.
…an intersection instead of a union. Note that the words "intersection" and "union" in TypeScript are misleading coming from a mathematical background.
Changes:
Related to #735, as this addresses some of the inconsistencies as well
Resolves #715