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

Replace const enum in .d.ts with string literal union #15

Closed
mz8i opened this issue Nov 2, 2024 · 4 comments
Closed

Replace const enum in .d.ts with string literal union #15

mz8i opened this issue Nov 2, 2024 · 4 comments

Comments

@mz8i
Copy link
Contributor

mz8i commented Nov 2, 2024

Thanks for the great library!
I'm having an issue related to #6 after using the suggested fix to #14

The problem is that importing const enum from a .d.ts file doesn't really work properly.

Consider the following fix to #14:

  csvSchema.cols.forEach((c) => {
    c.type = 's';
  });

This results in a TS error:
image

You can silence the error by forcing a cast to SchemaColumnType like this:

  csvSchema.cols.forEach((c) => {
    c.type = 's' as SchemaColumnType;
  });

but this is not recommended because it essentially removes the type checking, and if SchemaColumnType changes, or someone passes a wrong type value, TypeScript will not pick up the error:

image

I would suggest removing the const enum from the .d.ts and replacing it with a string literal union, which I think is the correct way to provide good DX here. Like this (I'll submit this exact suggestion as a PR):

export type SchemaColumnType = 
| /** String */ 's'
| /** Number */ 'n'
| /** Date */ 'd'
| /** JSON */ 'j'
| /** Boolean_1 */ 'b:1'
| /** Boolean_t */ 'b:t'
| /** Boolean_T */ 'b:T'
| /** Boolean_true */ 'b:true'
| /** Boolean_True */ 'b:True'
| /** Boolean_TRUE */ 'b:TRUE'
| /** Boolean_y */ 'b:y'
| /** Boolean_Y */ 'b:Y'
| /** Boolean_yes */ 'b:yes'
| /** Boolean_Yes */ 'b:Yes'
| /** Boolean_YES */ 'b:YES';

This will also give autocomplete:
image

Now, the only bit where this is not perfect is that the single-character strings are a bit less obvious than the names of the const enum elements. And currently, TypeScript sadly does not pick up the comments next to the union elements. But once it does (microsoft/TypeScript#38106), the comments should be included in IntelliSense.
But anyway, considering that the const enum approach is either incorrect or dangerous (depending on how you look at it), I think this is still a better alternative.

@leeoniya
Copy link
Owner

leeoniya commented Nov 3, 2024

hmm, this does appear to work tho?

image

@mz8i
Copy link
Contributor Author

mz8i commented Nov 3, 2024

Could you please share the tsconfig there, and the TS version?

EDIT: actually, upon looking at this again, I realised that on my side the syntax you show also doesn't get a type error - but it does result in a runtime error:

1 | (function (entry, fetcher)
    ^
SyntaxError: Export named 'SchemaColumnType' not found in module '/home/user/project/node_modules/udsv/dist/uDSV.cjs.js'.

I'll have to check if this doesn't have anything to do with Bun doing something weird re TypeScript. But could you please check if the code you show actually runs without an error? If yes, then I'd still be curious to see the configuration.

@leeoniya
Copy link
Owner

leeoniya commented Nov 3, 2024

i haven't checked, but it probably doesnt compile :)

i'm fine with the proposed change, since the strings are not int enum values and not completely nonsensical, and this doesnt create the mess that a regular enum would create at build time, though that may be irrelevant for a js lib where this is just a type declaration and the lib itself does not use it anyways.

image

@leeoniya leeoniya closed this as completed Nov 3, 2024
@leeoniya
Copy link
Owner

leeoniya commented Nov 3, 2024

(merged #16)

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

No branches or pull requests

2 participants