-
Notifications
You must be signed in to change notification settings - Fork 490
remove S3 SDK deps #1237
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
remove S3 SDK deps #1237
Conversation
@@ -13,7 +13,7 @@ | |||
</PropertyGroup> | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="AWSSDK.S3" Version="3.7.0" /> | |||
<PackageReference Include="Newtonsoft.Json" Version="13.0.1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to take a direct dependency on Newtonsoft.Json
because a lot of users prefer System.Text.Json
because it gives a greater cold start benefit. You can avoid referencing Newtonsoft
by using DataAnnotation
attributes instead of Newtonsoft's JsonProperty
attribute.
Here is an example for customizing the names without directly referencing Newtonsoft and supporting System.Text.Json
. https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.LexV2Events/LexV2Message.cs#L45
To avoid complications with System.Text.Json
you can drop netstandard2.0
from the Amazon.Lambda.S3.csproj file and have it only target netcoreapp3.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Thanks for the feedback!
Friendly bump @normj or @ashovlin if you don't mind taking a look at the changes 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pulled down the branch locally and confirm the existing S3 serialization test work. This will be a major version bump but user's migrating should have to do very little to switch. Most likely change a using statement.
The PR has been released in version 3.0.0 of the Amazon.Lambda.S3 package. Thank you for your work on this PR! |
Issue #, if available:
#1226
Description of changes:
S3Event.cs
to handle all properties correctlyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.