architect / functions

AWS Lambda Node runtime helpers for Architect apps
https://arc.codes
163 stars 38 forks source link

Add type definitions #524

Closed camjackson closed 2 years ago

camjackson commented 2 years ago

As discussed in https://github.com/orgs/architect/discussions/1346, this is the beginnings of adding stronger typing to the library. For now it's just the tables module, so I can get some feedback, and adjust the approach as needed.

Usage of these types would look something like this:

import arc from "@architect/functions";

type NoteTable = {
  pk: string;
  sk: string;
  title: string;
  body: string;
};

type UserTable = {
  pk: string;
  email: string;
};

type MyTables = {
  note: NoteTable;
  user: UserTable;
};

async function testingOutTheTypes() {
  const db = await arc.tables<MyTables>();

  // ❌ no such table
  await db.blah.delete();

  // ❌ no such attribute
  await db.note.delete({ blah: 'blah' });

  // ✅
  await db.note.delete({ pk: 'yay' });
};

Thoughts so far?

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

tbeseda commented 2 years ago

@camjackson this is great! I talked to @brianleroux and @macdonst and they agree this is a good approach. If you have capacity, I think it's worth investing more effort here. Thanks a lot for working on this.

camjackson commented 2 years ago

Ok great, thanks @tbeseda! I'll see if I can put some more time into it over the weekend.

camjackson commented 2 years ago

Ok I added a bunch of tests for the tables types, and improved the types as well along the way. I'm pretty happy with where that module is at now. There are still a bunch to do - I'll add a checklist to the top of this PR to make it easier to see progress.

camjackson commented 2 years ago

I wasn't super sure about the typings for the http module, especially the request and response types, so I copied those from the existing types. I did strip out stuff that I can see doesn't anymore though, like http.express and http.proxy.

camjackson commented 2 years ago

Ok @tbeseda, I think that's a completed first cut of this now. I've added types for everything on the arc object. It's based on a combination of:

I've used dtslint to add type tests for the tables, events, and queues objects, as those were the trickiest ones due to use of generics. The rest of the interfaces were pretty straightforward so I didn't feel like type tests would add much.

Happy to take feedback and revise as necessary :)

camjackson commented 2 years ago

Oh the other thing I'm not sure about is what the release path for this would be. Do we need to delete or otherwise deprecate the definitelytyped types? Given how different these types are, does this constitute a breaking change? 🤔

tbeseda commented 2 years ago

Hey @camjackson thanks for all the work here! It's looking good.

I did have to adjust ./types/tslint.json to get dtslint to pass locally.

{
  "extends": "@definitelytyped/dtslint/dtslint.json",
  "rules": {
    "interface-over-type-literal": false,
    "no-unnecessary-generics": false,
    "strict-export-declare-modifiers": false
  }
}

Is this the best approach to squashing these warnings?:

Error: /Users/tbeseda/dev/architect/functions/types/index.d.ts:9:6
ERROR: 9:6  strict-export-declare-modifiers  All declarations in this module are exported automatically. Prefer to explicitly write 'export' for clarity. If you have a good reason not to export this declaration, add 'export {}' to the module to shut off automatic exporting. See: https://github.com/Microsoft/dtslint/blob/master/docs/strict-export-declare-modifiers.md

/Users/tbeseda/dev/architect/functions/types/ws.d.ts:7:6
ERROR: 7:6  interface-over-type-literal      Use an interface instead of a type literal.
ERROR: 8:6  interface-over-type-literal      Use an interface instead of a type literal.
ERROR: 9:6  interface-over-type-literal      Use an interface instead of a type literal.

I think we could address the first one with another export {}?

camjackson commented 2 years ago

Oh whoops, I guess I forgot to run the lint check before the last push. That's what I get for coding late at night!

The first error is a reasonable one. I've just fixed that (rather than squashed it).

The other three are a bit silly, I don't really agree with that rule. I've turned it off.

Hopefully it goes green now!

tbeseda commented 2 years ago

Right on. The build passes locally, so I'm going to merge and I can make adjustments in a quicker feedback loop so we don't have to wait on GitHub back and forth :) -- I think I'll break dtslint into its own GH workflow so that CI can run in parallel. Then I can work on a release (I think it will be a minor version?) and look at the process for deprecating DefinitelyTyped. Thanks for sticking with this. I'm stoked to get rid of a some red squigglies in my editor and get that good autocomplete. Cheers!

camjackson commented 2 years ago

Yay! 🎉

Thanks for the feedback and for merging it! Can't wait to upgrade and start using it 🙂