feat: Migrate to TypeScript

[seb-cr]

This is a first iteration of Lambda Wrapper v2, which contains a number of breaking changes.

The key aims are:

• Migrate to a TypeScript-only codebase
• Reconsider how dependency injection should work so that dependency types can be inferred
• Fully document the new version
• Keep other changes to the package's API to a minimum

A good place to start reviewing this is probably the README, which provides examples of how the new wrapper works.

For a complete list of changes, please read the v2 migration guide. If you spot anything missing from there, please comment!

The tests are significantly affected, as you'd expect from changing how dependency injection works, and while going through them I took the opportunity to make improvements where I found them hard to understand. Currently test coverage is slightly below master (70.1% vs 75.7%).

In terms of what we'll do with this moving forward: v2 will first be published on the beta release channel. We can then test it out in one or two real-life applications to see how easy or difficult it is to work with, and continue to release improvements to the beta channel until we are satisfied that it's ready for wider use.

Jira: ENG-1710, [ENG-1725]
Closes #99

[pauldolden]

I've been giving this a more thorough read through this morning. The documentation and the migration guide are really helpful and well written, thanks for that.

As I've said previously I think this is a huge improvement on v1, even disregarding the obvious benefits of making this library type safe. The APIs are much more streamlined and offer more clarity than the previous method of DI. I think this syntax particularly:

export default lambdaWrapper.wrap(async (di) => {
  const request = di.get(RequestService);
  // ...
});

Is really well designed, and abstracts away some of the more clunky design decisions from v1.

I think the typing is largely fine, though if I recall correctly I believe this is something you said you'd like to iterate on and improve where possible, down the line.

The only real thought I had, in terms of design decisions, is the passing of a class directly to the di.get function. I can't put my finger on exactly why, but passing in a class as essentially an identifier, feels a little unintuitive. I think maybe, it could be that this could potentially leave room for some confusion over what instance of a class is actually being used within the Lambda.

This is not a hill I would die on, but I think it was a point potentially worth raising and discussing.

Another thought I had, as this is a wholesale rewrite of this library, would it be worthwhile to consider migrating to the AWS SDK v3 as a means of future proofing this library a little? I know there was some fairly considerable API changes so it may be the case that this is not plausible, but I thought it would be worth mentioning as we are unlikely to get many opportunities to implement this change down the line should Amazon start to phase out v2.

I'm not sure the extent of work this would create across other services, but as introducing a new version of Lambda Wrapper would constitute a breaking change any way, it might be worth considering doing this working in one fell swoop.

Again not something I'm calling out as a must have, but just a thought.

Your comments on test coverage are fair, but as this would initially be released into a beta version, I think that is something that could be improved fairly quickly if and when we encounter some limitations or bugs when we start testing this.

I think the pruning of more service specific logic (like the MarketingPreference model) is a good decision and keeping this package lean, but extensible is the best way of designing it. I do think it would be a good idea though, to look at some sort of companion service (or some other implementation), in order to keep frequently used services from being rewritten in a number of different repos, with slight variances in terms of implementation.

On the whole though, I think you've done a brilliant job with this and it will certainly help in laying the foundations for a more wide-spread use of TypeScript across our services. I noticed @corinja self-requested a review on this so it would be great to hear his thoughts on this too.

[seb-cr]

Thanks @pauldolden!

I think the typing is largely fine, though if I recall correctly I believe this is something you said you'd like to iterate on and improve where possible, down the line.

I'm struggling to remember now, but I think I ended up using a lot of any in the services to avoid substantial changes. One of the next steps after this PR will be to review some of those service methods and replace or remove the less type-safe ones.

The only real thought I had, in terms of design decisions, is the passing of a class directly to the di.get function. I can't put my finger on exactly why, but passing in a class as essentially an identifier, feels a little unintuitive. I think maybe, it could be that this could potentially leave room for some confusion over what instance of a class is actually being used within the Lambda.

Understandable. The class constructor approach did feel weird at first but it was the winner in terms of simplicity and I've grown fond of it since. Rather than an identifier, which is how v1 operates, consider it like this: di.get(MyClass) gets me an instance of MyClass. There's not really a question of which instance; all these service classes should be singletons. I'll check that I made this clear in the docs.

Another thought I had, as this is a wholesale rewrite of this library, would it be worthwhile to consider migrating to the AWS SDK v3 as a means of future proofing this library a little? I know there was some fairly considerable API changes so it may be the case that this is not plausible, but I thought it would be worth mentioning as we are unlikely to get many opportunities to implement this change down the line should Amazon start to phase out v2.

I'm not sure the extent of work this would create across other services, but as introducing a new version of Lambda Wrapper would constitute a breaking change any way, it might be worth considering doing this working in one fell swoop.

When updating other projects that use Lambda Wrapper, it might indeed make sense in terms of maintenance effort to do both at once.

Within Lambda Wrapper, I believe our use of AWS SDK is all "under the hood" so updating it should be only a patch release, which can be done anytime. It's therefore not something I'd look at before the 2.0.0 release (unless AWS deprecate it sooner than expected).

I do think it would be a good idea though, to look at some sort of companion service (or some other implementation), in order to keep frequently used services from being rewritten in a number of different repos, with slight variances in terms of implementation.

Agreed, and as you know I've been wanting to do this for a long time! Let's discuss separately in due course – could be a nice hackday project after RND.

[pauldolden]

All completely fair. As I mentioned on the call the other day, I didn't really have an awful lot to add on how you've gone about redesigning this. Those comments were largely points I thought worth discussing rather than a vehement desire to get changes made.

Like I said, I think this is all really good work and a big improvement on v1. Nice work! 🙂.

[corinja]

This is a great and substantial piece of work. I've read the readme files and skimmed the code and I can't suggest any changes, nor can I spot anything that needs to be added. I think it'll be good to start migrating services to use this Lambda Wrapper in order to spot any flaws, but I don't think there are any significant ones. Very impressive, overall.

[seb-cr]

After a hiccup with CI and my getting the commit message wrong for this PR, this is now released as version 2.0.0-beta.1 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release