headintheclouddev / typings-suitescript-2.0

TypeScript typings for SuiteScript version 2.0
MIT License
142 stars 89 forks source link

String values of enums #264

Open bendemeyer opened 6 months ago

bendemeyer commented 6 months ago

Is there a reason that the enums defined in this package don't have their actual string values assigned to them? I understand that it would be a bit weird from a philosophical standpoint, but I'm wondering if there's a practical reason to avoid it as well. I think there would be some practical benefits to providing those values, so if there are no practical drawbacks I think it's worth considering.

For example, I might want to pass a value from record.Type to a function that takes a string as an argument. Because the record.Type enum doesn't have values assigned in this package, I have to do that with as unknown as string, which is an inconvenience.

A less common but more impactful example would be if I wanted to type something such that it allowed string literals that are part of an enum. If the enum in question has values, this is doable with const thing: `${MyEnum}`;, but this can't be done with the enums as they're currently defined in this package.

If this would be desirable, I can submit a PR for it.

bendemeyer commented 6 months ago

Looking for other examples of this behavior, I noticed that there actually are some enums in this project that have values assigned. Assuming that's acceptable then, I'll try to put together a PR to add some of the missing ones.

MrRob commented 6 months ago

@bendemeyer Yea to be honest I'm not sure why they were set up the way they were. Not sure if @johnogle222 might know/remember? In my own code when I run into this I'll usually just cast record.type to String, like: String(record.type) instead of doing "as unknown as string". Slightly less annoying maybe?

ShawnTalbert commented 6 months ago

Correctness would be one reason not to specify the strings. As developers, we know that they are specific strings that correspond to those enumerations, but officially the enumeration is by what is provided - an abstraction...

On Tue, Jan 2, 2024, 5:24 AM Robin Mitchell @.***> wrote:

@bendemeyer https://github.com/bendemeyer Yea to be honest I'm not sure why they were set up the way they were. Not sure if @johnogle222 https://github.com/johnogle222 might know/remember? In my own code when I run into this I'll usually just cast record.type to String, like: String(record.type) instead of doing "as unknown as string". Slightly less annoying maybe?

— Reply to this email directly, view it on GitHub https://github.com/headintheclouddev/typings-suitescript-2.0/issues/264#issuecomment-1874022248, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKFNA56BKFFCTQB5NIN6I3YMQDBBAVCNFSM6AAAAABBFXDLAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZUGAZDEMRUHA . You are receiving this because you are subscribed to this thread.Message ID: <headintheclouddev/typings-suitescript-2.0/issues/264/1874022248@ github.com>

johnogle222 commented 6 months ago

Staying true to the abstraction and not making assumptions about what might change underneath was one big part of the motivation.

Not wanting to go through and discover all of the various actual string literals was another motivator. It's been, like, a decade now? But I'm pretty sure there were examples of situations where the enum value didn't exactly match the string[0], and I didn't want to encode absolutely false values into a typing library that might cause bad assumptions or even introduce bugs in code itself. The actual values could certainly be discovered if we wanted either through brute force or a script. And realistically NetSuite will likely not change any of these values without versioning the API. But sticking with the abstraction as it was documented felt like the "most right" thing to do when putting this together.

[0] I can only provide a hypothetical as it's been too long. But say there was a record.Type.CHECK enum documented and available. You couldn't just assume that the underlying string was "CHECK" or "check". It might be something like "NS_CHECK".

MrRob commented 6 months ago

Thank you for the wisdom @johnogle222 and @ShawnTalbert :)

bendemeyer commented 5 months ago

Staying true to the abstraction and not making assumptions about what might change underneath was one big part of the motivation.

This makes a ton of sense to me, broadly speaking, but I think the way this project is fundamentally set up undercuts it a bit. We don't have defined interfaces for each SuiteScript module, instead each one is represented by a file and the exports on that file represent the type of the module itself.

I'm guessing this was done so that we could map all the N/* modules to this project via tsconfig and have everything "just work," but it also creates complications like this where the module files need to export actual enums, as opposed to defining an interface that contains the appropriate enum types (similar to how the UserEventType enums are handled, since those don't have to be directly exported from a module).

I'm going to think on this a bit more. I can see a cleaner version of this where interface definitions are used for everything, and explicit module declarations handle the N/* mappings, but I'm not sure how to set that up in a way that's effective from a practical usage standpoint.