Skip to content

Conversation

@kostya-misura
Copy link

@kostya-misura kostya-misura commented Nov 30, 2017

Please fill in this template.

  • 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:

  • Provide a URL to documentation or source code which provides context for the suggested changes: Example from AWS below shows usage of string as result of Lambda execution. Judging from manual testing other primitive types are accepted as lambda execution result as-well. http://docs.aws.amazon.com/lambda/latest/dg/nodejs-prog-model-handler.html
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@dt-bot
Copy link
Member

dt-bot commented Nov 30, 2017

types/aws-lambda/index.d.ts

to authors (@darbio/aws-lambda-typescript @skarum @StefH/DefinitelyTyped @tobyhede @buggy @y13i @wwwy3y3 @OrthoDex @daniel-cottone). Could you review this PR?
👍 or 👎?

Copy link
Contributor

@daniel-cottone daniel-cottone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@typescript-bot
Copy link
Contributor

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

@armanio123 armanio123 merged commit 7377989 into DefinitelyTyped:master Dec 2, 2017
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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants