Skip to content

Conversation

@seb-cr
Copy link
Contributor

@seb-cr seb-cr commented Feb 29, 2024

This will bring the breaking changes on the beta branch into master, triggering the 2.0.0 release.

Note, in order for Semantic Release to correctly handle the transition and generate a meaningful changelog, we must create a merge commit on master rather than squashing the beta commits.

The purpose of this PR, apart from being the means of merging, is to track and review the changes between v1 and v2 and flag up any final issues or things we've forgotten. In terms of reviewing this PR, the diff is not that useful (and each change has already been reviewed separately). The commit list shows you the changes that are in the v2 beta releases. It's more of a "are we all happy to publish this release?" and "have we forgotten anything?" kind of review.

Jira: ENG-1710

seb-cr and others added 26 commits January 4, 2023 11:50
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

For a complete list of changes, please read the [v2 migration guide](https://github.com/comicrelief/lambda-wrapper/blob/beta/docs/migration/v2.md).

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.

BREAKING CHANGE: See the [v2 migration guide](https://github.com/comicrelief/lambda-wrapper/blob/beta/docs/migration/v2.md) for full details.
Currently config allows publishing from `master` only.
Currently this is a dev dependency and won't be installed with Lambda
Wrapper in another project. This is a problem because we re-export some
of its types, and TypeScript can't find them if the package hasn't been
installed.
So that someone looking at the `beta` branch gets instructions that they
can follow to install this version.

This will need reverting just before we merge `beta` into `master`.
Makes `RESPONSE_HEADERS` available for import from the package index.
This is useful if you want to change the default headers in your
responses, e.g. to change `Content-Type`.

The type is also relaxed to `Record<string, string>` to allow adding or
removing headers.

BREAKING CHANGE: the `Access-Control-Allow-Credentials` header value is
now the string `'true'` instead of Boolean `true`. This could affect
tests that rely on unstringified values.
Dependency-aware classes can now access the type of the Lambda Wrapper
config. This opens the door to using information from the config to
better type some methods.

As an example, our `SQSService` has many methods that take a queue name.
Previously these had to be type `string` and we relied on runtime checks
to ensure valid queue names were passed in. With access to the config
type, we can infer queue names from the keys of `config.sqs.queues`, so
this check can now be done at compile-time, _and_ we get IntelliSense
suggestions for queue names!

This is still a work in progress, although potentially useful and usable
enough in its current state to release to beta. See the updated
documentation for known issues.

There should be no breaking changes. All new type parameters are
optional. Unrecognised SQS queue names will raise compiler errors now,
however these should not exist in a working application.
Hidden because it was on a line join.
General housekeeping, and required to fix a vulnerability in `xml2js`.

I've done major updates for `jest` (which required regenerating
snapsnots in their new format) and `uuid` (no breaking changes relevant
to us). Other major updates require newer versions of Node.

Corresponds to #1097 which updated our v1.x dependencies.
The current type of `LambdaWrapper#wrap` requires handlers to return a
`Promise`. We should support synchronous handlers too.

No implementation change is required – they're already supported, just
disallowed by the type system.

Jira: [ENG-2732]
Epsagon is no longer operating. They took down their website on 3
October and backend services have been gradually removed, and since 10
October any service still making requests to Espagon servers faces
delays and Lambda timeouts.

We have now migrated to [Lumigo]. Their platform supports
[auto-tracing], allowing us to begin monitoring without deploying
updates to Lambda Wrapper, however we are missing our metrics and labels
which correspond to [Execution Tags] in Lumigo.

This PR makes the change to wrap handlers with Lumigo's tracer, and
updates our logger to use Execution Tags for metrics and labels. To
enable Lumigo tracing and tags, set `LUMIGO_TRACER_TOKEN` in your Lambda
environment to your [Lumigo token]. Note that if you have enabled
auto-tracing, this will be set automatically and tracing should "just
work".

There are a couple of other little things that need doing (e.g. removing
the `raiseOnEpsagon` flag) that I'll cover in separate PRs.

Jira: [ENG-2764]

[Lumigo]: https://lumigo.io/
[auto-tracing]: https://docs.lumigo.io/docs/serverless-applications#automatic-instrumentation
[Execution Tags]: https://docs.lumigo.io/docs/execution-tags
[Lumigo token]: https://docs.lumigo.io/docs/lumigo-tokens

BREAKING CHANGE: We no longer use Epsagon for monitoring. Lambda Wrapper
now supports [Lumigo](https://lumigo.io/) instead.
Adds validation to check that all dependencies have a unique name.

This follows on from [a discussion] with @zhibek about why we need to
turn off webpack's minification optimisation. TL;DR two dependency
classes can be minified to the same one-letter name, and since
dependencies are keyed by class name, this causes nasty runtime errors.

Lambda Wrapper will now be able to detect this situation and suggest
turning off minification, linking to the [readme note] of how to do
this for webpack.

As part of this, dependency classes will be deduplicated before being
instantiated. If a dependency is specified several times (unlikely but
possible) it will now be instantiated only once. Noting for completeness
in case we discover unexpected side effects down the line.

Jira: [ENG-2728]

[a discussion]: comicrelief/serverless-payments#614 (comment)
[readme note]: https://github.com/comicrelief/lambda-wrapper/tree/beta#notes
First step to replace our model classes with types.

Some advantages include:

- No boilerplate data class with getters and setters (and all the
corresponding tests)
- Less to maintain and smaller bundles
- IntelliSense for status values, even if Lambda Wrapper is used in
non-TypeScript projects

There are no longer run-time checks on the `status` value, but this is
not a regression. There was nothing to stop you bypassing those before,
e.g. by doing `statusModel.status = 'random string'`. Now, if you use
TypeScript, that is an error.

Jira: [ENG-2734]

BREAKING CHANGE: `StatusModel` and `STATUS_TYPES` are replaced with the
`ServiceStatus` and `Status` types
This is consistent with many other libraries (e.g. http, axios) and
makes it easier to work with HTTP headers.

The HTTP protocol states that header names are case-insensitive, so
currently when using `getAllHeaders` you must worry about the case of
the header name:

```js
const headers = request.getAllHeaders();
const contentType = headers['Content-Type'] || headers['content-type'];
// but what if someone sends us `Content-type`?
```

The existing `getHeader` method solved this by doing a case-insensitive
search of all keys. Now, you can also safely use the lowercase key:

```js
const headers = request.getAllHeaders();
const contentType = headers['content-type'];
```

Jira: [ENG-2735]

BREAKING CHANGE: Header keys returned by `getAllHeaders` are converted
to lowercase. Accessing headers through non-lowercase keys will yield
`undefined`, even if the header in the request is in the same case. Use
lowercase keys, or use `getHeader`.
Allows us to remove the non-null assertions, making this model more
type-safe.

This is unlikely to break any existing applications as this model is
intended for _received_ messages, which have all required fields.

Jira: [ENG-2733]

BREAKING CHANGE: `SQSMessageModel` will throw if required fields
(`MessageID`, `ReceiptHandle`, `Body`) are missing from the SQS message.
Further work to make `SQSMessageModel` more type-safe.

- Properties that have only a getter (no setter) are made readonly
- `body` type changed from `string` (incorrect) to `unknown`

Jira: [ENG-2733]
Change the name of the class to match the name exported from the package
index.

This should not be a breaking change because the class is the default
export of its module. Just in case there is some contrived case that
depends on the class's `name` attribute, we'll include this in the
v2.0.0 major release.

Jira: [ENG-2733]
The main point I wanted to add here is that this model is for _received_
messages. I've also added a bit of explanation about its intended use
with `SQSService`.

Jira: [ENG-2733]
Currently, sending an SQS message using `SQSService` does not throw an
error by default. This has caught us out a couple of times where we were
expecting a failure. The `failureMode` parameter was added to opt in to
throwing errors, to avoid introducing a breaking change in v1.

In v2 we'll throw errors by default, and you'll have to opt _out_ of
this behaviour.

BREAKING CHANGE: `SQSService#publish` throws by default instead of
handling errors. To maintain old behaviour, you can pass `"catch"` in
the `failureMode` parameter.
Makes 'beta' up-to-date with 'master' following some dependency updates
in Lambda Wrapper v1.

Most of these changes are irrelevant for 'beta'. I kept only the
`@sentry/node` update.
Includes major version bump for `axios`. Everything else is minor/patch.
v29.1.2 introduces a breaking change, dropping support for Node 14.
They don't follow SemVer in order to keep their major version number
aligned with Jest.

This repo needs updating to Node 18 (at a minimum), but this should not
be a blocker for the 2.0.0 release.
@seb-cr
Copy link
Contributor Author

seb-cr commented Mar 6, 2024

I've noticed there are some issues with types. I'll mark this as draft until they are resolved.

(If anyone else has found bugs, comment here or add as a subtask to the Jira ticket)

@seb-cr seb-cr marked this pull request as draft March 6, 2024 16:14
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]
@seb-cr
Copy link
Contributor Author

seb-cr commented Mar 26, 2024

Both type issues resolved – they were down to incorrect usage and the quirks of IntelliSense in JS projects. In TS projects the issues do not exist.

Just to keep track of additional changes since my last comment:

Once these are all merged I'm happy to proceed.

seb-cr added 2 commits March 28, 2024 10:13
Adds some basic config validation at runtime, which will help avoid
problems in projects that are not using TypeScript. In particular, we
will now throw an error if a dependency does not extend
`DependencyAwareClass`.

Not marking this as a separate breaking change because bad configs will
already cause compile-time errors in TypeScript.

Jira: [ENG-3207]
@seb-cr seb-cr marked this pull request as ready for review March 28, 2024 10:35
@corinja corinja removed the request for review from 7777akhil March 28, 2024 15:13
corinja
corinja previously approved these changes Apr 2, 2024
To be merged just before #1195, so that the docs in the 2.0.0 release do
not tell you to install from the beta channel.
@seb-cr seb-cr merged commit 7500846 into master Apr 2, 2024
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.

3 participants