redis / redis-om-node

Object mapping, and more, for Redis and Node.js. Written in TypeScript.
MIT License
1.18k stars 80 forks source link

SchemaDefinition - Improved TypesScript support (enhancement) #205

Closed jr-litmot closed 1 year ago

jr-litmot commented 1 year ago

Hi,

I'm using Redis OM 0.4.2. If we were to add generic type support to Schema and SchemaDefinition, we could create a schema like this:

import { Entity, Schema } from 'redis-om'

export interface Foo extends Entity {
  id: number
  description: string
}

export const fooSchema = new Schema<Foo>('foo', {
  id: { type: 'number' },
  description: { type: 'string' }
})

The advantage gained would be autocompletion and validation of the schema definition key names. (Suppose the Foo entity interface is defined and used elsewhere in the project. If Foo's properties are changed without updating the schema definition [or vice-versa], a compile error is thrown.)

Note that we wouldn't lose the ability to create schema instances in the current way. This would still work:

const barSchema = new Schema('bar', {
  id: { type: 'number' },
  description: { type: 'string' }
})

Proposed implementation:

Change SchemaDefinition in definitions.ts to:

export type SchemaDefinition<T> = Record<Exclude<keyof T, keyof Entity>, FieldDefinition>

Then change Schema in schema.ts like so:

export class Schema<T> {

  #schemaName: string
  #fieldsByName: Record<string, Field> = {}
  #definition: SchemaDefinition<T>
  #options?: SchemaOptions

  /**
   * Constructs a Schema.
   *
   * @param schemaName The name of the schema. Prefixes the ID when creating Redis keys.
   * @param schemaDef Defines all of the fields for the Schema and how they are mapped to Redis.
   * @param options Additional options for this Schema.
   */
  constructor(schemaName: string, schemaDef: SchemaDefinition<T>, options?: SchemaOptions) {
    this.#schemaName = schemaName
    this.#definition = schemaDef
    this.#options = options

    this.#validateOptions()
    this.#createFields()
  }
  ...

If it seems acceptable, I'm willing to submit a PR.

guyroyse commented 1 year ago

That looks quiet nice. I would certainly welcome such a PR. Thanks!

jr-litmot commented 1 year ago

I tried making this change, but I realized the Exclude involving Entity doesn't quite produce the desired result, I think due to Entity having some computed property names or perhaps the [key: string] from EntityData.

The following seems to work well, but without the ability to use an entity as the generic (which diminishes the usefulness):

export type SchemaDefinition<T> = Record<keyof T, FieldDefinition>

It seemed like a good idea at the time, but it would need a better implementation. I'll go ahead and close this issue for now.