dynamodb-toolbox icon indicating copy to clipboard operation
dynamodb-toolbox copied to clipboard

Remove the need for `as const` and develop an attributes builder

Open ThomasAribart opened this issue 1 year ago • 4 comments

To implement type inference in v0.4, I used the as const statement mainly to prevent breaking changes, but it makes the parsing of attributes quite complex and resource intensive. It's clear that the API had not been thought out with TypeScript in mind, and I was kind of fitting a circle in a square there.

Besides, there are some features that are cumbersome or simply not doable this way:

  • Ensuring that a partitionKey is always defined
  • Typing the input & output of the default and transform functions
  • Typing the dependsOn keyword

And some parts of the API are simply confusing and error prone:

  • Defining arrays containing numbers ? (I think default function is enough)
  • Defining map vs alias attribute ? (I think alias is enough)

Anyway, before looking forward for nested fields validation, enums etc... I think a complete overhaul of the attributes definition system would be nice. For instance, we can get rid of the as const by using functions or classes, which allow to store input values as generic types:

class StringAttribute<R extends boolean, D extends undefined | string | (args:any) => string> {
  constructor({ required, default }: { required: R, default: D }) {
   ...
  }
}

const myAttributes = {
  someString: new StringAttribute({ required: true, default: "value" })
  ...
}

Then, we could apply a generic type like GetInput<typeof myAttributes> to recursively obtain the GetInput of the entity rather than awkwardly attach an Attributes generic type to the Entity class.

We can also imagine:

  • Separating the key attributes in a different property rather than using a partitionKey: true keyword / PartitionKey class
  • Using recursive typing for MapAttribute.

An even better idea I think, would be to take inspiration from https://github.com/colinhacks/zod and use some kind of builder pattern:

const myAttributes = partitionKey({ key: 'pk', type: 'string' })
  .stringAttribute({ key: 'someString', default: ({ pk }) => ...  }) // <= argument can be typed from previous fn call output

An example from Twitter:

image

What do you think ?

ThomasAribart avatar Jul 27 '22 23:07 ThomasAribart

Hello @ThomasAribart

The current typescript implementation is unusable because of the type complexity which mainly comes from the API, as you noted.

I agree the API needs to be reworked completely. @jeremydaly, does this fit in your vision of the project? Is this something that could be done before v1?

I would like to push this further as I really believe this project should be typescript first. How can I help?

gabrielcolson avatar Aug 02 '22 06:08 gabrielcolson

@jeremydaly @gabrielcolson I started working on this, check out https://github.com/jeremydaly/dynamodb-toolbox/pull/305/files

ThomasAribart avatar Aug 04 '22 08:08 ThomasAribart

Hey all, I think an overhaul of Table and Entity constructors makes a lot of sense. The original API was designed back in 2019, so TypeScript support wasn't really on the radar. The goal of this project has always been to make things as developer friendly as possible, so a simple set of builder classes would certainly fit that mold. Let me look over @ThomasAribart's PR and make some comments on the semantics.

jeremydaly avatar Aug 04 '22 16:08 jeremydaly

And some parts of the API are simply confusing and error prone:

  • Defining arrays containing numbers ? (I think default function is enough)
  • Defining map vs alias attribute ? (I think alias is enough)

I 100% agree with this. The map vs alias feature was to enable defining attributes bi-directionally, but definitely adds confusion. I would be in favor of defining the entity name first and then the attribute it maps to as a configuration.

The arrays functionality was for composite keys, a practice that is rather outdated at this point (though popular at the time). Computed values would work just fine for this.

jeremydaly avatar Aug 04 '22 16:08 jeremydaly