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

Export enum instead of type for simple C-like enums #23

Closed
JakubKoralewski opened this issue Jul 16, 2021 · 8 comments
Closed

Export enum instead of type for simple C-like enums #23

JakubKoralewski opened this issue Jul 16, 2021 · 8 comments

Comments

@JakubKoralewski
Copy link

When Rust's enums could be represented with TypeScript's enums I think they should. So for example an enum like:

#[derive(serde::Serialize, ts_rs::TS)
enum Color {
	#[serde(rename="R")
	Red,
	Blue,
	Green
}

should end up as

export enum Color {
	Red="R",
	Blue,
	Green
}

Maybe even const enum? Should probably be optional with an attribute on the enum.

Instead of currently it being: export type Color = "R" | "Blue" | "Green";

@gyzerok
Copy link

gyzerok commented Oct 29, 2021

I would personally consider trade-offs when making this decision.

Output size

When you use enum you actually generate code while types are "free". For the example you mentioned output will be:

var Color;
(function (Color) {
    Color["Red"] = "R";
    Color[Color["Blue"] = void 0] = "Blue";
    Color[Color["Green"] = void 0] = "Green";
})(Color || (Color = {}));

Now if we take into account that modelling data with enums in Rust is a common practice just imagine how much extra code will be in your application. If we talk about frontend application, bundle size actually matter.

There is also a memory footprint which is small, but good to keep in mind:

{
  "0": "Red",
  "1": "Blue",
  "2": "Green",
  "Red": 0,
  "Blue": 1,
  "Green": 2
}

Probably const enums might work well, however I am unsure if they have some other significant downsides, which makes sense to consider.

Type safety

Since the whole purpose of this library is to generate TypeScript types, I think we can safely assume then any project using it will use TypeScript and not JavaScript.

And from TypeScript perspective enums do not give you any extra safety. In fact every error that you would like to avoid with enums you can avoid with union of literals.

In my experience people usually want enums because when they come from JavaScript they feel like using just strings is unsafe. However in practice there are no drawbacks here.

Conclusion

I would suggest to keep the current behavior.

@JakubKoralewski
Copy link
Author

JakubKoralewski commented Oct 31, 2021

Probably const enums might work well,

Yes they should nullify your case about memory usage.

I am unsure if they have some other significant downsides, which makes sense to consider.

I've had some issues with sharing const enums across projects, can't recall the details.

My thought process is: let's make it possible to do this.

When you use enum you actually generate code

Maybe that's what the user wants. You can achieve some nice behavior by indexing into the enum object, or iterating through all the keys etc.

Feel free to keep the behavior as is, I think having the option to switch to enum should be available though.

@NyxCode
Copy link
Collaborator

NyxCode commented Nov 8, 2021

I do agree that having the option to generate one or the other is better than not having it.
I think we need a way to configure the macro not only through attributes, but also through a config file which applies to all invocations. Otherwise, users code will drown in attributes to configure the output for every invocation. This is currently really tricky because of incremental compilation, but I'll take a look at it. If anyone wants to give that ago, please do!

@barel-mishal
Copy link

barel-mishal commented Nov 4, 2023

Hi, it seems I encountered a similar issue. However, after reading the discussion here, I've realized that it might be more cost-effective to eliminate my use of enums in my code. I'm uncertain about the necessity of supporting enums in JavaScript/TypeScript.

That said, I'd like to provide some context. In my project, I've been using a lot of enums. Lately, I've been transitioning to Rust to benefit from better maintainability, fewer bugs, and a superior testing environment. This led me to the project "ts-rs", which I personally appreciate and use extensively. Occasionally, I wish it had better compatibility with serde when I utilize types from other crates.

Regarding enums, while I don't believe "ts-rs" needs to support JavaScript enums, it should offer documentation and links to relevant discussions. After discovering this discussion, I began transitioning my enums to types using "ts-rs". I must say the results have been promising with less code to manage and similar logic to before, with only a few changes.

// using types from ts-rs
export const getFeedBackText = (
    feedback: Feedback
  ) => {
    switch (feedback) {
 
      case 'SignificantlyBelowRange':
        return `some text...`;
  
      case 'SlightlyBelowRange':
        return `some text...`;
  
      case 'WithinRange':
        return `some text...`;
  
      case 'SlightlyAboveRange':
        return `some text...`;
  
      case 'SignificantlyAboveRange':
        return `some text...`;
  
    }
  
  }
  // using enum!
  export const getFeedBackText = (
    feedback: Feedback, 
  ) => {
  
    switch (feedback) {
  
      case Feedback.SignificantlyBelowRange:
        return `some text...`;
  
      case Feedback.SlightlyBelowRange:
        return `some text...`;
  
      case Feedback.WithinRange:
        return `some text...`;
  
      case Feedback.SlightlyAboveRange:
        return `some text...`;
  
      case Feedback.SignificantlyAboveRange:
        return `some text...`;
    }
  }

As you can observe, the code has barely changed and retains its previous types. Another example demonstrates using a function to return an enum type.

// using types from ts-rs
export const getFeedback = (intake: number, range: AdjustedBounds): Feedback => {
    if (intake < range.lowerBound) return 'SignificantlyBelowRange';
    if (intake < range.min) return 'SlightlyBelowRange';
    if (intake >= range.min && intake <= range.max) return 'WithinRange';
    if (intake > range.max && intake <= range.upperBound) return 'SlightlyAboveRange';
    return 'SignificantlyAboveRange';
}

// using enum
export const getFeedback = (value: number, range: AdjustedBounds) => {
    if (value < range.lowerBound) return Feedback.SignificantlyBelowRange;
    if (value < range.min) return Feedback.SlightlyBelowRange;
    if (value >= range.min && value <= range.max) return Feedback.WithinRange;
    if (value > range.max && value <= range.upperBound) return Feedback.SlightlyAboveRange;
    return Feedback.SignificantlyAboveRange;
}

In the second example, I explicitly returned the type of Feedback to ensure consistent type accuracy. This was the primary logic difference necessary to maintain the same type safety for this use case.

I hope to see improved documentation and more insights on handling unsupported types. While this topic is mentioned, I find it to be a frequent and challenging issue for newcomers wanting to integrate this project into their workflow.

Thank you for creating this project!

@hpapier
Copy link

hpapier commented Dec 12, 2023

Hi :)

While I think reconsidering the use cases of enums in Typescript is good (leading to stronger typing, less code generation, etc), it still has some main benefits, the main one I see so far is the fact that an enum serves also as a constant of values, that you can iterate over. It is then used to type and to centralise information. For me both cases should be covered, transforming enums to types or to plain JS enums.

Thank you for the project! :)

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Jan 26, 2024

TS enums are generally considered to be really bad (as pointed out by @gyzerok, they generate extra JS and are not very type-safe) and serde doesnt really support any conversions that would make them easy to use out of the box (though it can probably be done with serde-repr). In general type unions, wether it is a union of strings or a discriminated union are the preferred way to go, as pointed out by @barel-mishal, so I don't think support for TS enums is planned for this crate.

We have been working on improving the use of Rust enums by allowing them to be flattened but I think that's all we will be doing in regards to enums.

Would like @NyxCode to confirm this first, but I'm leaning towards closing this as not planned

@escritorio-gustavo
Copy link
Contributor

If this feature is really something people need, please make a PR after #206 is merged

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 26, 2024

Agreed, let's close this for now. If there ends up being lots of interest in the feature, we should probably do some refactorings first, especially because I'm not sure how it'd interact with enum flattening.

@NyxCode NyxCode closed this as completed Jan 26, 2024
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 a pull request may close this issue.

6 participants