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

AoT Support? #262

Closed
martincostello opened this issue Nov 20, 2023 · 6 comments
Closed

AoT Support? #262

martincostello opened this issue Nov 20, 2023 · 6 comments

Comments

@martincostello
Copy link

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.

Copy link

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

@stoiveyp
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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.

@martincostello martincostello closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2023
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

No branches or pull requests

3 participants