Skip to content

Conversation

@polovi
Copy link
Contributor

@polovi polovi commented Dec 30, 2017

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the [readme]
  • Avoid [common mistakes]
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

@mhegazy
Copy link
Contributor

mhegazy commented Jan 3, 2018

@typescript-bot
Copy link
Contributor

We've gotten sign-off from a reviewer 👏. A maintainer will soon review this PR and merge it if there are no issues. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for contributing to DefinitelyTyped!

@typescript-bot typescript-bot added Other Approved This PR was reviewed and signed-off by a community member. Merge:LGTM labels Jan 3, 2018
@mhegazy mhegazy merged commit 68f6a78 into DefinitelyTyped:master Jan 3, 2018
simonbuchan added a commit to simonbuchan/DefinitelyTyped that referenced this pull request Feb 7, 2018
No tests added for the new types yet, I'll get on that tomorrow.

Motivation
---

The connection between the trigger event and the result types can be
confusing sometimes - for example `CloudFormationCustomResourceResponse`
is *not* returned from the lambda, but instead should be sent to the
*Request `ResponseUrl`. This PR tries to help by providing pre-baked
`Handler` types for each of the triggers (that have types already).

There's also a few niggles with naming and the handler signature that
have been bothering me for a while.

Changes
---

This PR:

- Adds defaulted generic parameters to the existing `Handler` and
  `Callback` types. This increases the minimum TS version to 2.3.

- Makes the `callback` parameter to `Handler`s "required" - this
  probably originally was meant to represent the Node 0.10 runtime,
  which did not pass a callback parameter, but that is no longer
  selectable when creating or editing a Lambda and makes the normal,
  recommended usage harder. In case anyone is confused, this doesn't
  break any code: it is required to be provided when being called, your
  handlers don't need to declare it. I'm not removing the legacy node 0.10
  runtime methods `context.done()` etc., since they still work.

- In the spirit of DefinitelyTyped#21874, make the default result type for
  `Handler`, `Callback` `any`, since the only requirement is
  that it be `JSON.stringify()`able, and there are things like
  `.toJSON()`, etc. so there is no meaningful type restriction
  here.

- Revert the definition changes of DefinitelyTyped#22588, which changed the `Handler`
  result types to `void | Promise<void>`, which is misleading: Lambda
  will not accept or wait for a promise result (it by default waits
  for the node event loop to empty). This is not a breaking change for
  code that does return promises, since a `void` result type is
  compatible with any result type, even with strict function types.
  (Which is why the tests still pass.)

- Adds `FooHandler` and `FooCallback` types for all `FooEvent`s and
  `FooResult`s, where possible - sometimes these don't match up.

- Renamed, with aliases to the old name, various types to align them: in
  particular `ProxyResult` -> `APIGatewayProxyResult`.

- `CloudFrontResult` (not the same type as `CloudFrontResponse`!)
  was missing for some reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Other Approved This PR was reviewed and signed-off by a community member.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants