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

Re-export runtime types in server generated crates #1909

Merged
merged 9 commits into from
Jan 17, 2023
Merged

Re-export runtime types in server generated crates #1909

merged 9 commits into from
Jan 17, 2023

Conversation

82marbag
Copy link
Contributor

Re-export commonly used types from aws-smithy-http-server so that customers don't have to depend on that crate.

We did not agree to a full list yet. This is a possible starting point. We could also either add all types from the server runtime crate.

Motivation and Context

Closes #1506

Description

Add a server module that re-exports some of the types from the runtime crate.

Testing

Generated the Pokemon service.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Re-export commonly used types from aws-smithy-http-server so that customers don't have
to depend on that crate.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag 82marbag requested a review from a team as a code owner October 26, 2022 14:26
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>

rustCrate.withModule(
RustModule.public(
"server",
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively here we could

pub use aws_smithy_http_server as server;

Which would allow us to preserve the root crate docs but would mean that we re-export everything from the aws-smithy-http-server crate.

Copy link
Contributor

@hlbarber hlbarber Oct 27, 2022

Choose a reason for hiding this comment

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

The conclusion of a discussion about this was: no, we don't want to do a blanket re-export for now. But we should ensure we preserve the layout between codegen items and aws-smithy-http-server items so that if we reconsider it will be a non-breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't server a confusing name? What about calling the module aws_smithy_http_server to better signal where we are re-exporting from? Not a blocker TBH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to hide where re-exports are from and allow us to re-export also from other runtime crates possibly, probably we shouldn't signal the name of the crate

Copy link
Contributor

@LukeMathWalker LukeMathWalker Jan 17, 2023

Choose a reason for hiding this comment

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

It feels like server is clear from context and it buys us a bit of flexibility, as @82marbag was saying.

Copy link
Contributor

Choose a reason for hiding this comment

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

Allright! Thanks for the comments.

Comment on lines 32 to 35
##[doc(hidden)]
pub use #{SmithyHttpServer}::instrumentation;
##[doc(hidden)]
pub use #{SmithyHttpServer}::proto;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of doing a #[doc(hidden)] re-export? I might be missing something obvious 🤔

Copy link
Contributor

@hlbarber hlbarber Oct 26, 2022

Choose a reason for hiding this comment

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

It means that #1886 would only have to remove the #[doc(hidden)] rather than making decisions on what to include in this module or not. Also, people using these modules for "nightly aws-smithy-http-server" would still be able to remove aws-smithy-http-server from their dependency list.

If #[doc(hidden)] is transitive and re-exporting something #[doc(hidden)] means the re-export is also hidden then this isn't necessary - I'm unsure if this is the case or not.

The decision to do this is made automatically if we choose to #1909 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

If #[doc(hidden)] is transitive and re-exporting something #[doc(hidden)] means the re-export is also hidden then this isn't necessary - I'm unsure if this is the case or not.

This was my expectation, hence the question.
I tested it and I found that:

  • Re-exporting a #[doc(hidden)] module gives you a #[doc(hidden)] module, so no need to mark the rexport as #[doc(hidden)];
  • Any #[doc(hidden)] item inside the re-exported module remains #[doc(hidden)].

Copy link
Contributor

Choose a reason for hiding this comment

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

Code:

pub use dep::hello as hey;
pub use dep::my_hidden_module as hey_from_hidden;

Dependency code:

pub mod hello {
    #[doc(hidden)]
    struct MyHiddenStruct;
}

#[doc(hidden)]
pub mod my_hidden_module {
    struct MyVisibleStruct;
}

Screenshot of the docs:
image
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know, we can drop the #[doc(hidden)] from these.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti added the server Rust server SDK label Dec 20, 2022
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@82marbag 82marbag enabled auto-merge (squash) January 11, 2023 10:49
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

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.

Re-export runtime type in server generated crates
5 participants