Skip to content
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

Add boxed method to service builder #1840

Merged
merged 5 commits into from
Oct 25, 2022
Merged

Add boxed method to service builder #1840

merged 5 commits into from
Oct 25, 2022

Conversation

hlbarber
Copy link
Contributor

@hlbarber hlbarber commented Oct 11, 2022

Motivation and Context

As a result of the Service Parameterized Routers change, the {ServiceName::layer} modifies the type of the service. This has the benefit of avoiding the heap but means that types can build up with several applications of layer.

For users who wish to erase the types via Route::new (Box::new in disguise), we provide a boxed method. This can be seen as an opt-in reversal to the Service Parameterized Routers change.

Description

  • Add boxed method to the service builder.
  • Add reasonable default S to the service builder.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber marked this pull request as ready for review October 11, 2022 22:39
@hlbarber hlbarber requested a review from a team as a code owner October 11, 2022 22:39
@hlbarber hlbarber enabled auto-merge (squash) October 17, 2022 14:19
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

/// Applies [`Route::new`](#{SmithyHttpServer}::routing::Route::new) to all routes.
///
/// This has the effect of erasing all types accumulated via [`layer`].
pub fn boxed<B>(self) -> $serviceName<#{SmithyHttpServer}::routing::Route<B>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this method go in an extension trait in a runtime crate?
Bringing this up because I feel that less generated code is usually better.

Copy link
Contributor

@crisidev crisidev Oct 20, 2022

Choose a reason for hiding this comment

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

I actually believe the opposite.. generated code is easier to manage and release than runtime code.. the more we codegenerate, the easier is for customers to consume as it is not tied to crates releases.

Copy link
Contributor Author

@hlbarber hlbarber Oct 21, 2022

Choose a reason for hiding this comment

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

If this was a standalone function I think it might fit better in the rust-runtime crates but given that this is a small method on a generated struct I think it makes sense to leave it here for now. If we did this via extension trait the customer would have to import the trait to use the method which may be confusing.

It is possible though: we'd need to add a base trait with the layer method on it, impl that onto PokemonService via codegen, and then include this boxed method extension to that trait.

Maybe we should draft out a heuristic to help in this kind of decision making?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should figure out guidelines to decide whether to put stuff in runtime vs codegen!

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I'd lean towards putting this method in the code generated struct. It's a better developer experience than having to import a single-use extension trait just to call it.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber enabled auto-merge (squash) October 25, 2022 12:09
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber merged commit 6b3e311 into main Oct 25, 2022
@hlbarber hlbarber deleted the harryb/box-service branch October 25, 2022 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants