Skip to content

Make lambda::Context a first class part of the Handler api #233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 30, 2020

Conversation

softprops
Copy link
Contributor

@softprops softprops commented Jun 27, 2020

Issue #, if available:

Description of changes:

This makes LambdaCtx a second argument passed to handler types

This makes for a bit more consistency with handler apis in other runtimes like node, python, and ruby

go is an outlier because of its native context pattern idiom

See conversation here

update:

This change also renames lambda::LambdaCtx to lambda::Context

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

async fn actual(#arg_name: #arg_type) #ret {
#body
}
async fn actual(#event_name: #event_type, #context_name: #context_type) #ret #body
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: I ditched the branches because I noticed a warning emitted about needless braces. we debugging the result I could see that body actually included braces so the result looked like async fn actual(foo: XXX, bar: XXX) -> Ret { { ... } } hence I omitted the extra braces to avoid potential user confusion with rust warnings


if is_http(&args) {
quote_spanned! { input.span() =>
use lambda_http::lambda::LambdaCtx;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not actually sure why these were here before but they cased duplicate import errors as handlers no import this type themselves in their function signatures

@softprops
Copy link
Contributor Author

softprops commented Jun 27, 2020

Something that made me itch a bit is the name lambda::LambdaCtx A suggested alternative might be lambda::Context. If users have other Context types in their code one can always rename with use lambda::Context as LambdaCtx

@cakekindel
Copy link

I've always really enjoyed type names that are very concise, and lean on the module to preserve the context of the type (lambda::Context)

@softprops
Copy link
Contributor Author

@cakekindel I'm with you :)

@davidbarsky
Copy link
Contributor

I think we should push customers (and in examples) to write lambda::Context wherever possible. The current naming is somewhat unfortunate/an oversight.

@softprops
Copy link
Contributor Author

Happy to add that here or after a merge. Which ever comes first

@softprops softprops changed the title Make LambdaCtx a first class part of the handler api Make lambda::Context a first class part of the Handler api Jun 28, 2020
@davidbarsky
Copy link
Contributor

Eh, it’s more of a value statement. Happy to merge this thing in first.

@softprops
Copy link
Contributor Author

I ended out doing a quick find and replace

@davidbarsky
Copy link
Contributor

I think we should get rid of the INVOCATION_CTX wholesale (cc: @insanitybit) but I'll merge this in for now.

@davidbarsky davidbarsky merged commit ae3ce1d into awslabs:master Jun 30, 2020
@softprops
Copy link
Contributor Author

I think we should get rid of the INVOCATION_CTX wholesale

me too #237

@seanpianka
Copy link
Contributor

FWIW, @softprops it was a breaking change going from #231 (ed3fd16) to #233 (ae3ce1d), at least for those whose Cargo.toml entry for this crate was tracking with branch = "master".

Here's my error:

error[E0593]: function is expected to take 2 arguments, but it takes 1 argument
   --> src/cmd/lambda/main.rs:91:25
    |
53  | async fn entrypoint(req: lambda_http::Request) -> Result<impl IntoResponse, AsyncError> {
    | --------------------------------------------------------------------------------------- takes 1 argument
...
91  |     lambda::run(handler(entrypoint)).await?;
    |                         ^^^^^^^^^^ expected function that takes 2 arguments
    |
   ::: /Users/sean/.cargo/git/checkouts/aws-lambda-rust-runtime-7c865cce90132439/ae3ce1d/lambda-http/src/lib.rs:131:19
    |
131 | pub fn handler<H: Handler>(handler: H) -> Adapter<H> {
    |                   ------- required by this bound in `lambda_http::handler`
    |
    = note: required because of the requirements on the impl of `lambda_http::Handler` for `fn(http::request::Request<lambda_http::body::Body>) -> impl std::future::Future {entrypoint}`

@softprops
Copy link
Contributor Author

@seanpianka this is expected.

Note depending on master holds different guarantees than depending on a release version.

There are no guarantees on on API stability on the master branch as it represents active development.

We are hoping to pin down what the next release version looks like which, for those depending on the latest release version, should expect a number of breaking changes

@seanpianka
Copy link
Contributor

seanpianka commented Aug 24, 2020

I've since pinned the crate at ed3fd16 using the following in Cargo.toml:

lambda_http = { git = "https://github.com/awslabs/aws-lambda-rust-runtime/", rev = "ed3fd16" }

As it seems the guarantees from depending on master are not suitable for my needs.

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.

4 participants