feat(typescript): getInternal type safety + other type improvements

[m-radzikowski]

Fixes #1023

So far, getInternal() returned any type:

middy/packages/util/index.d.ts

Lines 36 to 39 in a080562

	declare function getInternal (
	variables: any,
	request: middy.Request
	): Promise<any>

With those changes, the information on the data stored in the internal storage is passed through and extended by middlewares (as type). The getInternal() function validates passed variables (if they occur in the enriched type) and returns typed object (with passed-in keys and their matching value types).

The data type in the internal storage is passed in the new generic TInternal type.

Working example with SSM:

middy()
  .use(
    ssm({
      fetchData: {
        lorem: '/lorem',
        ipsum: '/ipsum',
      },
    }),
  )
  .before(async request => {
    const p = await getInternal(["lorem"], request); // ✅ output type: { lorem: unknown; }
    p.lorem // ✅
    p.ipsum // ✅ error - property does not exist 
    p.nonexisting // ✅ error - property does not exist 

    await getInternal(["lorem", "nonexisting"], request); // ✅ error -  TS2322: Type  "nonexisting"  is not assignable to type  "lorem" | "ipsum"
  })

WIP

To do:

• support other getInternal() variables argument variants - with true and keys mapping object (this currently causes tsd tests to fail)
• getInternal() output parameters have type unknown at the moment - the actual type of variable is available in the TInternal type, probably can be extracted to return proper parameter types
• check SSM logic for adding parameters to TInternal - I've simply added all keys, but similar code for the Context type has more logic - check it
• implement adding keys to TInternal for all other middleware
  • appconfig
  • dynamodb
  • rds-signer
  • s3
  • s3-object-response
  • secrets-manager
  • service-discovery
  • ssm
  • sts

[m-radzikowski]

@lmammino here is the basic implementation :) I think most of the heavy job is done, although there are still a few things to improve in getInternal() types. And then updating all middlewares to add types to TInternal, which should be fairly easy.

I can't commit to spending more time on this right now - this part took me quite a moment... If you or anyone else want to continue the work - feel free to. Otherwise, I can try to get back to it, but don't know how soon (or late).

[lmammino]

Hey @m-radzikowski, thanks for doing this! It's definitely a great starting point. I'll try to pick up the remaining work from here. I'll ask you more questions/help if I get stuck ;)

[m-radzikowski]

Hey @m-radzikowski, thanks for doing this! It's definitely a great starting point. I'll try to pick up the remaining work from here. I'll ask you more questions/help if I get stuck ;)

Sure! Don't hesitate to ping me if needed.

[lmammino]

I spent (quite) a bit of time trying to figure out how to improve the getInternal type definition:

This is what I came up with:

type Flattened<T> = T extends Array<infer U> ? Flattened<U> : T;

// single variable
declare function getInternal<TInternal extends Record<string, unknown>, TVars extends keyof TInternal | string> (
  variables: TVars,
  request: middy.Request<any, any, any, any, TInternal>
): Promise<{
  [P in TVars]: (TVars extends keyof TInternal ? TInternal[TVars] : unknown)
}>
// array of variables
declare function getInternal<TInternal extends Record<string, unknown>, TVars extends Array<keyof TInternal | string>> (
  variables: TVars,
  request: middy.Request<any, any, any, any, TInternal>
): Promise<{
  [P in Flattened<TVars>]: (P extends keyof TInternal ? TInternal[P] : unknown)
}>
// mapping object
declare function getInternal<TInternal extends Record<string, unknown>, TMap extends Record<string, keyof TInternal | string>> (
  variables: TMap,
  request: middy.Request<any, any, any, any, TInternal>
): Promise<{
  [P in keyof TMap]: (TMap[P] extends keyof TInternal ? TInternal[TMap[P]] : unknown)
}>
// all variables (with true)
declare function getInternal<TInternal extends Record<string, unknown>> (
  variables: true,
  request: middy.Request<any, any, any, any, TInternal>
): Promise<{
  [P in keyof TInternal]: TInternal[P]
}>

It doesn't look pretty but it seems to work for the most part.

There are still a few advanced corner cases that are not properly handled yet.

For instance:

• The remapping function can do nested remapping if using a dot syntax such as property.subproperty. This will currently be typed as unknown
• If internal contains a promise, the return value should be the resolved version of that promise (right now it keeps the promise).

I am not sure if these additional improvements are worth it, so I wouldn't stress too much about them. This seems already like a big step forward.

Thoughts?

[gegham1]

@lmammino

• TVars extends keyof TInternal | string this does not bring any value, can be simplified to just string
• Promise<{ [P in keyof TInternal]: TInternal[P] }> can be replaced with just Promise<TInternal>

Also, I noticed that in mapping version TMap is a Map<string... and returned object keys are the keys from TMap, which can be anything. Is this a desired behavior?
e.g.
// will inferred as { b: number }
let obj = await getInternal({b: 'ac'}, { resp: {ac: 4} });

[wtfzambo]

@lmammino alright if possible I made the code even uglier but I managed to achieve a bunch of things:

1. Get rid of overloads
2. Solve your problem with nested keys in TInternal (if I understood properly what the fuck was the issue in the first place)

I'm not too proud of this but it's all I could achieve in one day.

There's one caveat in the "mapping" case: TVars must be passed either as an object literal, OR the variable must be declared as const.

type Flattened<T> = T extends Array<infer U> ? Flattened<U> : T;

// finds TKey in TInternal, recursively, even if it's a deeply nested one
type Resolve<TInternal, TKey> = TKey extends keyof TInternal
  ? TInternal[TKey]
  : TKey extends string
  ? {
      [P in keyof TInternal]: TInternal[P] extends object
        ? Resolve<TInternal[P], TKey>
        : never;
    }[keyof TInternal]
  : never;

// for when TVars is an object, find all keys in TInternal that match the values in TVars
type DeepResolve<TInternal, TVars> = {
  [P in keyof TVars]: Resolve<TInternal, TVars[P]>;
};

// all possible shapes that TVars can have (this is based on your declarations)
type TVars<T, K> = true | keyof T | (keyof T)[] | Record<string, K>;

// all possible return values that getInternal() can produce (also based on your declaration)
type TGetInternalResults<TVars, TInternal> =
  TVars extends keyof TInternal
  ? { [P in TVars]: TInternal[TVars] }
  : TVars extends Array<keyof TInternal | string>
  ? {[P in Flattened<TVars>]: P extends keyof TInternal ? TInternal[P] : unknown}
  : TVars extends object
  ? DeepResolve<TInternal, TVars>  // <-- this is for the mapped case
  : TInternal;  // <-- this is for the "true" case


// "K extends string" seems useless but it isn't. Without it, the (nested) mapped case doesn't work
// I removed "TInternal extends Record<string, unknown>" part because it doesn't do anything useful in my examples.
declare function getInternal<TInternal, K extends string, T extends TVars<TInternal, K>>(
  variables: T,
  request: middy.Request<any, any, any, any, TInternal>
): Promise<TGetInternalResults<T, TInternal>>

// example of the nested mapped case
const request = {
  internal: {
    a: 'asd',
    b: 123,
    c: true,
    d: {
      e: new Promise(() => {})
    },
  },
};

let obj = getInternal({foo: 'e', bar: 'a'}, request);
obj.then((value) => value.foo);  // value.foo inferred as Promise<unknown>
obj.then((value) => value.bar);  // value.bar inferred as to string

[lmammino]

After a good pairing session with @jfet97 yesterday and some reflection hours today, I think I finally have a solution that provides the correct types for util.getInternal including:

• Resolves promises correctly
• Resolves object paths correctly
• Applies correct normalisation of keys

I needed to create a bunch of utility types so I ended up creating a type-utils.d.ts file to keep them isolated (and testable).

Huge thanks to @jfet97 but also to @gegham1 & @wtfzambo for providing very useful (and appreciated) suggestions and alternative approaches!

[jfet97]

It's been a pleasure @lmammino , I'm glad you managed to complete the work 😄

[lmammino]

The tests for the SSM middleware where failing because of infinite recursion type when using the new typing system for getInternal. The SSM middleware uses the type-fest package's JsonValue type.

In order to address this issue I did 2 things:

1. I added limited recursion to the DeepAwaited type utility (@jfet97 if you could check that I'd be super happy!). This should also speed up a little the type inference.
2. I removed type-fest entirely. As an alternative, I came up with an alternative approach that allows us to specify the expected types for every parameter very explicitly. I updated the docs as part of this PR as well to illustrate this idea. Here's an extract:

Usage with TypeScript

Data in SSM can be stored as arbitrary JSON values. It's not possible to know in advance what shape the fetched SSM parameters will have, so by default the fetched parameters will have type unknown.

You can provide some type hints by leveraging the ssmParam utility function. This function allows you to specify what's the expected type that will be fetched for every parameter.

The idea is that, for every parameter specified in the fetchData option, rather than just providing the parameter path as a string, you can wrap it in a ssmParam<ParamType>(parameterPath) call. Internally, ssmParam is a function that will return parameterPath as received, but it allows you to use generics to provide type hints for the expected type for that parameter.

This way TypeScript can understand how to treat the additional data attached to the context and stored in the internal storage.

The following example illustrates how to use ssmParam:

import middy from '@middy/core'
import { getInternal } from '@middy/util'
import ssm, {ssmParam} from '@middy/ssm'

const handler = middy((event, context) => {
  return {}
})

let globalDefaults = {}
handler
  .use(
    ssm({
      fetchData: {
        accessToken: ssmParam<string>('/dev/service_name/access_token'), // single value (will be typed as string)
        dbParams: ssmParam<{user: string, pass: string}>('/dev/service_name/database/') // object of values (typed as {user: string, pass: string})
      },
      cacheExpiry: 15 * 60 * 1000,
      cacheKey: 'ssm-secrets'
    })
  )
  // ... other middleware that fetch
  .before(async (request) => {
    const data = await getInternal(
      ['accessToken', 'dbParams', ],
      request
    )
    // data.accessToken (string)
    // data.dbParams ({user: string, pass: string})
  })

What do you think of this idea?
CC: @willfarrell , @m-radzikowski , @naorpeled

[naorpeled]

The tests for the SSM middleware where failing because of infinite recursion type when using the new typing system for getInternal. The SSM middleware uses the type-fest package's JsonValue type.

In order to address this issue I did 2 things:

1. I added limited recursion to the DeepAwaited type utility (@jfet97 if you could check that I'd be super happy!). This should also speed up a little the type inference.
2. I removed type-fest entirely. As an alternative, I came up with an alternative approach that allows us to specify the expected types for every parameter very explicitly. I updated the docs as part of this PR as well to illustrate this idea. Here's an extract:

Usage with TypeScript

Data in SSM can be stored as arbitrary JSON values. It's not possible to know in advance what shape the fetched SSM parameters will have, so by default the fetched parameters will have type unknown.
You can provide some type hints by leveraging the ssmParam utility function. This function allows you to specify what's the expected type that will be fetched for every parameter.
The idea is that, for every parameter specified in the fetchData option, rather than just providing the parameter path as a string, you can wrap it in a ssmParam<ParamType>(parameterPath) call. Internally, ssmParam is a function that will return parameterPath as received, but it allows you to use generics to provide type hints for the expected type for that parameter.
This way TypeScript can understand how to treat the additional data attached to the context and stored in the internal storage.
The following example illustrates how to use ssmParam:

import middy from '@middy/core'
import { getInternal } from '@middy/util'
import ssm, {ssmParam} from '@middy/ssm'

const handler = middy((event, context) => {
  return {}
})

let globalDefaults = {}
handler
  .use(
    ssm({
      fetchData: {
        accessToken: ssmParam<string>('/dev/service_name/access_token'), // single value (will be typed as string)
        dbParams: ssmParam<{user: string, pass: string}>('/dev/service_name/database/') // object of values (typed as {user: string, pass: string})
      },
      cacheExpiry: 15 * 60 * 1000,
      cacheKey: 'ssm-secrets'
    })
  )
  // ... other middleware that fetch
  .before(async (request) => {
    const data = await getInternal(
      ['accessToken', 'dbParams', ],
      request
    )
    // data.accessToken (string)
    // data.dbParams ({user: string, pass: string})
  })

What do you think of this idea? CC: @willfarrell , @m-radzikowski , @naorpeled

Sounds good to me 😎
Will start reviewing the code now!

[naorpeled]

LGTM!

Great job guys!

[socket-security]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@types/aws-lambda	8.10.122	None	+0	137 kB	types

[lmammino]

I think I covered all the points in the TODO list. This is not ready for a final review :)