search

timheuer / alexa-skills-dotnet

An Amazon Alexa Skills SDK for .NET
MIT License
547 stars 108 forks source link

AoT Support? #262

Closed martincostello closed 11 months ago

martincostello commented 12 months ago

I've just updated an Alexa skill of mine to .NET 8 which uses this library.

Even though I know it isn't currently compatible with AoT, I thought I'd compile it locally with PublishAot=true to see how much would need to be changed to enable it when I run it in AWS Lambda.

For this library, the main source of trim warnings I get seems to come from the use of Newtonsoft.Json.

Are there any plans to change the library to be AoT-compatible, and if not, would a PR to replace the usage of Newtonsoft.Json with System.Text.Json to be able to make things AoT compatible be welcome?

To be honest, I imagine it's a non-trivial piece of work, but I thought it was worth asking the question.

If not, at some point I might just implement the specific subset of the Alexa Skill API that I actually use within my app so that it is AoT compatible.

github-actions[bot] commented 12 months ago

👋 Hi there! Thanks for your contribution to the project by posting your first issue!

stoiveyp commented 12 months ago

Morning! Thanks for reaching out.

Changing JSON library is something that has come up before. Originally System.Text.Json was missing crucial functionality, custom converters on Interfaces was the big one. After that was updated I did (apparently a year ago now!) get to a point where I could create a version of the main library with System.Text.Json that got all the Alexa tests passing. (code is here: https://github.com/stoiveyp/Alexa.NET.SystemTextJson)

The issue is that we have over a dozen NuGet packages that are built on the Newtonsoft types, and would break if we switched the main library. If we run two separate packages with limited initial support - then we have to be careful keeping changes in sync and making sure users are clear on what package they're installing.

Maybe something for us to discuss at some point @timheuer?

timheuer commented 12 months ago

Yeah I would consider this a big potential break for a set of folks so we'd want to manage it well. It also would be a dependency bump I think beyond netstandard -- which might alienate a set of consumers. At a minimum coordinating with all your plugins @stoiveyp for sure...as an all-in-one release. AOT is a good motivating factor, but the change makes me nervous for the benefit if I had to be honest.

martincostello commented 12 months ago

Interesting - I wasn't aware there was a big set of extensions.

Sounds like the churn required to support it would be a lot of work for probably not much gain? If so, happy for you folks to close this.

stoiveyp commented 12 months ago

Thanks @martincostello - for reference if you look at the readme file in the repo, there is a list of the extensions we have for the project. We try and maintain parity for all the APIs the Alexa team have released.

martincostello commented 11 months ago

I'm going to close this as it doesn't sound like there's any appetite to support this, and for my own skill I've re-implemented the serialization parts of top of System.Text.Json to remove the Newtonsoft.Json dependency at runtime so I can publish the application with AoT enabled.