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

[BUG] Integer enums that don't have explicit values set produce broken types #154

Open
1 task done
daogrady opened this issue Feb 6, 2024 · 2 comments
Open
1 task done
Labels
bug Something isn't working good first issue Good for newcomers keepalive Will not be closed by Stale bot

Comments

@daogrady
Copy link
Contributor

daogrady commented Feb 6, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Nature of Your Project

JavaScript, TypeScript

Current Behavior

When defining an integer enum without explicitly assigning values to the keys, the generated types will contain syntax errors (see repro for a sample model).

These enums can be compiled without issue, although the output looks a bit unexpected:

$ cds repl
> await cds.load('foo.cds')
{
  definitions: {
    Foo: {
      kind: 'type',
      type: 'cds.Integer',
      enum: { a: {}, b: {}, c: {} }
    }
  },
  meta: { creator: 'CDS Compiler v4.5.1', flavor: 'inferred' },
  '$version': '2.0'
}

Expected Behavior

Expected output is not clear either. I would have expected to have the keys enumerated in order, starting from 0 (or 1?). The type should be set accordingly.
Log a fatal error prompting the user to explicitly define values. See comment below. Type could be just number iff we want to make sure the run produces syntactically correct types even in this case.

Steps To Reproduce

  1. create a file:
// foo.cds
type Foo: Integer enum { a; b; c; }
  1. run:
cds-typer foo.cds
  1. check the generated index.ts file and find:
export const Foo = {
  a: undefined,
  b: undefined,
  c: undefined,
} as const;
export type Foo =   // <- syntax error

Environment

- **OS**: Mac
- **Node**: irrelevant
- **npm**: irrelevant
- **cds-typer**: 0.16
- **cds**: irrelevant

|  | https://github.com/<your/repo> |
|:---------------------- | ----------- |
| Node.js                | v19.9.0     |
| @sap/cds               | 7.6.0       |
| @sap/cds-compiler      | 4.5.1       |
| @sap/cds-dk            | -- missing  |
| @sap/cds-dk (global)   | 7.5.1       |
| @sap/eslint-plugin-cds | 2.6.5       |
| @sap/cds-mtxs          | 1.14.0      |
| @cap-js/cds-types      | -- missing  |

Repository Containing a Minimal Reproducible Example

No response

Anything else?

This is probably a rather easily fixable issue by iterating over the entries of integer enums and coalescing the values with their respective index.

https://github.com/cap-js/cds-typer/blob/main/lib/components/enum.js

@daogrady daogrady added bug Something isn't working new good first issue Good for newcomers keepalive Will not be closed by Stale bot labels Feb 6, 2024
@daogrady
Copy link
Contributor Author

After conferring with @bugwelle we came to the conclusion that we should not assume an implicit numbering for missing values at this point in time. The compiler may at some point introduce an implicit numbering that could start with either 0 or 1 and would not be resilient to adding new enum values between existing values. So anything we could use would probably produce breaking changes.

While the compiler currently only produces a warning, this should warrant an error from cds-typer, as this is something we currently simply can not provide definitive types for.

@daogrady
Copy link
Contributor Author

To consider: duplicate values?

@daogrady daogrady removed the new label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers keepalive Will not be closed by Stale bot
Projects
None yet
Development

No branches or pull requests

1 participant