giraffe-fsharp / Giraffe

A native functional ASP.NET Core web framework for F# developers.
https://giraffe.wiki
Apache License 2.0
2.11k stars 267 forks source link

Looks like whole module `SystemTextJson` disappeared, but no mention in change log #606

Closed abelbraaksma closed 1 month ago

abelbraaksma commented 1 month ago

When upgrading from 6.0 to 7.0, code stopped compiling, specifically, lines like these are now invalid:

let serializationOptions = SystemTextJson.Serializer.DefaultOptions

This seems to be caused by this 557a2a6d069ed3511e0a12857958b33734aff924. A public module that disappeared there. While I understand the need for breaking changes, I would have hoped it would make it to the Release Notes (or better, be marked Deprecated for some time first), but the last related change is actually about introducing this module: https://github.com/giraffe-fsharp/Giraffe/blob/ec722467ade83cef51b7cdba32909ab9533e5af2/RELEASE_NOTES.md?plain=1#L127

Either way, I guess you cannot just willy nilly get this back, so I'll just update the repositories that use lines like the above. But if possible, maybe next time add this to the release notes?

Pointing it out here in case somebody else runs into mysterious errors when updating.

image

64J0 commented 1 month ago

Hi @abelbraaksma, thanks for opening this issue, and sorry for the lack of information at the RELEASE_NOTES.

[...] or better, be marked Deprecated for some time first

This is something I'll consider for the future updates, thanks for raising this point.

abelbraaksma commented 1 month ago

Hi @64J0, Giraffe is an amazing package, no worries, these things sometimes slip through.

I guess we can close this, unless you want to update the release notes retroactively?

64J0 commented 1 month ago

I guess we can close this, unless you want to update the release notes retroactively?

I'll do this. It must be helpful for other people too.

64J0 commented 1 month ago

Do you think this is enough? https://github.com/giraffe-fsharp/Giraffe/pull/607. I'll update the release notes at the GitHub release after this PR is merged.

abelbraaksma commented 1 month ago

@64J0 I see you already merged it. Personally, I'd call out breaking changes (in my projects I simply do that with an all caps 'BREAKING' and place it on top), and update the main Readme with a note on the version update and a pointer to an upgrade document.

For instance, it could have instructions like 'replace SystemTextJson with Json. This is not trivial to discover otherwise. I only found out by looking at the commit history, which shouldn't be necessary.

I'm not sure if there are other functions or modules changed or removed.

64J0 commented 1 month ago

Ack, later I'll open a new PR tackling those points. Thanks for the heads-up!

aspnetde commented 1 month ago

Re: https://github.com/giraffe-fsharp/Giraffe/pull/563#issuecomment-2244975320

I know resources are limited here and writing documentation is not much fun. But having a release page with a list of new functionality, bug fixes, and, most importantly, breaking changes, would be really appreciated. Given the version bump to 7.0 I already suspected such changes and seeing the change of the JSON handling I knew this would probably cause trouble, so I gave it a try, ran into a crash and downgraded back to 6.4 (and put it on my backlog). So, all fine for me. Others might be more surprised, though.

Thank you for the continued work on this!

64J0 commented 1 month ago

What do you think @abelbraaksma and @aspnetde? https://github.com/giraffe-fsharp/Giraffe/pull/608

abelbraaksma commented 1 month ago

I responded in the PR, @64J0. Thanks for the quick action!

64J0 commented 1 month ago

Do you think we can already close this issue @abelbraaksma?

64J0 commented 1 month ago

Closing this issue now, please let me know if you think there's something else to do.