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

Add support for enum to "typeorm-typescript/enforce-column-types" #4

Open
SimonSimCity opened this issue May 21, 2024 · 4 comments
Open

Comments

@SimonSimCity
Copy link

The rule currently reports at a property defined as:

export const POSSIBLE_VALUES = ['v1', 'v2', 'v3'] as const;

@Entity('my_data')
export class MyData {
  @Column({ type: 'varchar', nullable: true, enum: POSSIBLE_VALUES })
  myEnumProp?: POSSIBLE_VALUES | null;
}

... and provides the option to change the type of myEnumProp to string | null, which is wrong, because I have the option enum enabled. See: https://typeorm.io/entities#enum-column-type

It should not report on this column. It should instead report on a property, where the property enum is enabled, but the type is number or string:

export const POSSIBLE_VALUES = ['v1', 'v2', 'v3'] as const;

@Entity('my_data')
export class MyData {
  @Column({ type: 'varchar', nullable: true, enum: POSSIBLE_VALUES })
  myEnumProp?: string | null;
}
@daniel7grant
Copy link
Owner

I was thinking about supporting enums, but the main problem with ESLint is that it cannot resolve types, only "tokens". That's why there is no support for JSON validation, because we cannot resolve the shape, only the name. Since we cannot get the type behind the name, I have a hard time imagining a way this would work.

  // ESLint only sees, this with no idea what POSSIBLE_VALUES is, outside the name
  @Column({ type: 'varchar', nullable: true, enum: POSSIBLE_VALUES })
  myEnumProp?: typeof POSSIBLE_VALUES | null;

The only way I can think of this working would be if you specify the array of possible values directly in the enum, but I think it would kinda defeat its purpose:

@Entity('my_data')
export class MyData {
  @Column({ type: 'varchar', nullable: true, enum: ['v1', 'v2', 'v3'] })
  myEnumProp?: 'v1' | 'v2' | 'v3' | null;
}

This way, you have to maintain the two list separately, which would cause more type mismatches, rather than less.

Another idea would be adding some logic in a first pass to store all arrays or enums, and resolve them to their values manually, given they are usually in the same file. However, this would not only make this check much slower, but it would probably be full of holes, and would eventually end up rebuilding the type checker from scratch. For example even in the example above, we should resolve the array and also a typeof T operator in the field. And it still wouldn't handle people putting the types in another variable or putting it in another file, which are completely valid usecases.

The only idea I could do is removing validation for enums, since they will usually come from external variables anyway or at most add a validation that enum: ARRAY has the type typeof ARRAY. Feel free to brainstorm, if you have any idea.

@SimonSimCity
Copy link
Author

I've seen rules where the rule must know the type. Take for example the rule no-floating-promises - here's the code. I'm not into building eslint rules, so I can't quite yet tell you which part is for evaluating if a variable or a function-expression is of type Promise, but this looks very promising to me.

@daniel7grant
Copy link
Owner

daniel7grant commented Jun 14, 2024

Thank you Simon, you are correct, I was unaware, but typescript-eslint added a feature called Typed Rules, which allows introspection of the TypeScript types. I looked at this feature, and it would make this lint much better if it could resolve the types, but so far I didn't find this trivial to implement.

If I manage to make the library work with typed rules, I'd believe it would be simple to add your feature requests.

@daniel7grant
Copy link
Owner

I added types in the new release, however I wasn't able to make this work. I tried to pull out some information about the enum values, but even with as const it didn't seem to give me the literal keys. It might be possible to complete this, but I cannot sink more time into it right now.

If anyone can contribute this as a PR, I will review and merge it, but it is unlikely that I will write this.

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