Skip to content

fix: Improve type of configure config param#1196

Closed
seb-cr wants to merge 1 commit into
betafrom
ENG-3163/fix-configure-config-type
Closed

fix: Improve type of configure config param#1196
seb-cr wants to merge 1 commit into
betafrom
ENG-3163/fix-configure-config-type

Conversation

@seb-cr
Copy link
Copy Markdown
Contributor

@seb-cr seb-cr commented Mar 14, 2024

This allows IntelliSense to suggest the dependencies key (and any other existing config keys) when you are entering the config parameter.

The TMoreConfig type, which is used to track the addition of arbitrary keys to the Lambda Wrapper config object, was not constrained and so could be inferred as any.

The type of the config param therefore defaulted to Partial<TConfig> & any. This is equivalent to any, and IntelliSense cannot make suggestions about keys that may be available.

By requiring TMoreConfig to extend Partial<LambdaWrapperConfig>, we still allow any object but tell the type system about expected optional keys, so IntelliSense can suggest them.

Jira: ENG-3163

This allows IntelliSense to suggest the `dependencies` key (and any
other existing config keys) when you are entering the config parameter.

The `TMoreConfig` type, which is used to track the addition of arbitrary
keys to the Lambda Wrapper config object, was not constrained and so
could be inferred as `any`.

The type of the `config` param therefore defaulted to
`Partial<TConfig> & any`. This is equivalent to `any`, and IntelliSense
cannot make suggestions about keys that may be available.

By requiring `TMoreConfig` to extend `Partial<LambdaWrapperConfig>`, we
still allow any object but tell the type system about expected optional
keys, so IntelliSense can suggest them.

Jira: [ENG-3163]
@seb-cr seb-cr marked this pull request as draft March 18, 2024 10:47
@seb-cr
Copy link
Copy Markdown
Contributor Author

seb-cr commented Mar 18, 2024

This fix solves this problem but I've discovered that it breaks the SQS queue name type in SQSService. Need to find an alternative solution, and I'll also look at adding some tests for types so that we can find out immediately if we accidentally break an indirectly related type.

@seb-cr
Copy link
Copy Markdown
Contributor Author

seb-cr commented Mar 22, 2024

I finally tracked this back to a difference in how IntelliSense handles TS and JS files. In TS files, this bug does not exist.

// test.ts
import lambdaWrapper from '@comicrelief/lambda-wrapper'

const lw = lambdaWrapper.configure({})
// IntelliSense says:
// configure<unknown>(config: Partial<LambdaWrapperConfig & WithSQSServiceConfig>): LambdaWrapper<LambdaWrapperConfig & WithSQSServiceConfig>

config has the correct type and IntelliSense will suggest the dependencies and sqs keys.

But if you rename the file to .js, suddenly the type changes to any:

// test.js
import lambdaWrapper from '@comicrelief/lambda-wrapper'

const lw = lambdaWrapper.configure({})
// IntelliSense says:
// configure<any>(config: any): LambdaWrapper<any>

So, I do not believe this is a bug in Lambda Wrapper, after all!

Closing this PR.

@seb-cr seb-cr closed this Mar 22, 2024
@seb-cr seb-cr deleted the ENG-3163/fix-configure-config-type branch March 22, 2024 14:41
seb-cr added a commit that referenced this pull request Mar 25, 2024
First attempt at adding tests for some of the types used in Lambda
Wrapper v2.

In #1196 I tried to fix one problem and inadvertently caused another
more serious problem. I caught this during manual QA, but this is
precisely why automated tests are so valuable. Some of the type-related
features of Lambda Wrapper are complex enough that we should have their
behaviour defined in a test suite.

About these tests: they do not "run", as such, and instead the types are
checked statically by the TypeScript compiler. All assertions happen at
the type system level. Test failures will be flagged by a type error on
the failing assertion. I think it still makes sense to organise these
tests using `describe` and `it` to give structure and context to the
assertions.

The docs for [expect-type](https://github.com/mmkal/expect-type#readme)
are useful. I tried various packages (`ts-expect`, `tsd`) and found
`expect-type` to provide the most expressive API and it doesn't need any
particular setup.

So far I've written tests for only `di.get` and `SQSService`, as this
covers most of the interesting stuff and is where I've been seeing
problems – there's enough here to fail following the changes in #1196.
It would also be good to cover things relating to Lambda Wrapper
configuration, however this was not as straightforward as I hoped so I'm
leaving it for now.

Jira: [ENG-3188]
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

Successfully merging this pull request may close these issues.

1 participant