Improved TypeScript types #3030

New Issue

lionel-rowe · 2023-11-22T08:13:59Z

lionel-rowe commented

2023-11-22 08:13:59 +00:00

Overall the TypeScript types seem quite loose compared to the documentation and implementation. For example:

export function read(data: any, opts?: ParsingOptions): WorkBook;

In reality, it seems like data must be one of string | ArrayBuffer | Uint8Array | Buffer (where string can be either a file name, a base64-encoded string, a binary encoded string, or a plaintext UTF-8 string) and can further be narrowed down by passing opts.type. The exact logic for this narrowing looks to be a little complex, so maybe simply typing as string | ArrayBuffer | Uint8Array | Buffer would suffice (from testing, ArrayBuffer and Uint8Array both work nicely without passing any options), and at least it avoids ambiguity as to whether Files, Blobs, DataViews, etc. are supported.

It'd also be helpful to document the default values for the various flags in Sheet2JSONOpts within the d.ts file. Currently, only UTC has a documented default.

Finally, sheet_to_json with no type parameter passed might be possible to narrow further by making Sheet2JSONOpts generic, as it seems (for example) sheet_to_json(ws, { header: 1 }) is guaranteed to return (string | number | boolean | undefined)[][], rather than any[][].

There are probably various other similar issues in the TypeScript types, so my main question is, are PRs accepted for this kind of stuff? And if so, should I just edit types/index.d.ts directly? I notice there's also xlsx.flow.js — what's the relationship between the Flow types and the TS types? Is one automatically generated from the other?

Overall the TypeScript types seem quite loose compared to the documentation and implementation. For example: ```ts export function read(data: any, opts?: ParsingOptions): WorkBook; ``` In reality, it seems like `data` must be one of `string | ArrayBuffer | Uint8Array | Buffer` (where `string` can be either a file name, a base64-encoded string, a binary encoded string, or a plaintext UTF-8 string) and can further be narrowed down by passing `opts.type`. The exact logic for this narrowing looks to be a little complex, so maybe simply typing as `string | ArrayBuffer | Uint8Array | Buffer` would suffice (from testing, `ArrayBuffer` and `Uint8Array` both work nicely without passing any options), and at least it avoids ambiguity as to whether `File`s, `Blob`s, `DataView`s, etc. are supported. In addition, `sheet_to_json`'s `Sheet2JSONOpts` parameter's `header` property is given as `"A"|number|string[]`, whereas the [docs](https://docs.sheetjs.com/docs/api/utilities/array#array-output) have it as `1 | "A" | string[]`, and the implementation seems to also allow `0 | 2 | 3` (presumably this used to be an enum-like `0 | 1 | 2 | 3` in older versions and it's kept in for legacy reasons?) It'd also be helpful to document the default values for the various flags in `Sheet2JSONOpts` within the d.ts file. Currently, only `UTC` has a documented default. Finally, `sheet_to_json` with no type parameter passed might be possible to narrow further by making `Sheet2JSONOpts` generic, as it seems (for example) `sheet_to_json(ws, { header: 1 })` is guaranteed to return `(string | number | boolean | undefined)[][]`, rather than `any[][]`. There are probably various other similar issues in the TypeScript types, so my main question is, are PRs accepted for this kind of stuff? And if so, should I just edit `types/index.d.ts` directly? I notice there's also `xlsx.flow.js` — what's the relationship between the Flow types and the TS types? Is one automatically generated from the other?

sheetjs commented

2023-11-22 19:21:07 +00:00

In general, PRs are appreciated :) You can edit the types file directly -- it has no relation to the xlsx.flow.js script.

The Sheet2JSONOpts type can be changed. There may have been an issue with older versions of TS where {header: 1} would match the type { header: number; } but not { header: 1; }.

Likewise, any sort of @default or JSDoc comment would be useful.

Overall, the historical context is that we aim to maintain backwards compatibility even when the rest of the ecosystem breaks.

To set the expectations, we shouldn't require NodeJS or DOM or ES6 types.

ArrayBuffer and Uint8Array are part of the Typed Arrays proposal, not included in the default types (they were added in ES2017)

Buffer historically was a separate class from Uint8Array. Now it is a subclass, so in theory it shouldn't be needed, but explicitly pulling Buffer requires the NodeJS types.

In general, PRs are appreciated :) You can edit the types file directly -- it has no relation to the `xlsx.flow.js` script. The `Sheet2JSONOpts` type can be changed. There may have been an issue with older versions of TS where `{header: 1}` would match the type `{ header: number; }` but not `{ header: 1; }`. Likewise, any sort of `@default` or JSDoc comment would be useful. --- Overall, the historical context is that we aim to maintain backwards compatibility even when the rest of the ecosystem breaks. To set the expectations, we shouldn't require NodeJS or DOM or ES6 types. `ArrayBuffer` and `Uint8Array` are part of the Typed Arrays proposal, not included in the default types (they were added in ES2017) `Buffer` historically was a separate class from `Uint8Array`. Now it is a subclass, so in theory it shouldn't be needed, but explicitly pulling `Buffer` requires the NodeJS types.

lionel-rowe referenced this issue

2023-12-10 09:16:08 +00:00

Improve types for read[File], write[File], sheet_to_json #3041

Sign in to join this conversation.