fsprojects / FsHttp

A lightweight F# HTTP library by @SchlenkR and @dawedawe
https://fsprojects.github.io/FsHttp/
Apache License 2.0
424 stars 42 forks source link

Guard semantic versioning for major changes using binary compat checks in .fsproj #194

Open abelbraaksma opened 2 months ago

abelbraaksma commented 2 months ago

I couldn't find this documented, not in a release-notes, and not in the PR where it happened (#167). Several functions were updated with extra arguments (namely: cancellationToken) that cause existing code to break at compile time (in my source code in 100+ places, it's a non-trivial effort to fix):

image

I understand the need to update public functions, but introducing backwards compatibility should be done with the biggest restraints, and prevented if at all possible. My suggestion would be to link to an update page where the changes are highlighted, or first have a version that marks functions as obsolete (ObsoleteAttribute), which you can use to explain users what to use instead, going forward.

I know, it is OSS and I shouldn't be too fuzzy about all of this, we're all volunteers out here :). In fact, I'm currently in a similar situation with F#'s TaskSeq (see: https://github.com/fsprojects/FSharp.Control.TaskSeq/issues/179, https://github.com/fsprojects/FSharp.Control.TaskSeq/issues/167 and https://github.com/fsprojects/FSharp.Control.TaskSeq/discussions/188). Meaning, I need to support cancellation tokens but ideally without introducing backward compat issues.

Rants aside, I really appreciate this library, it has made our lives a lot easier!!! ❤️

abelbraaksma commented 2 months ago

I see a similar thing happened with StartingContext, which disappeared in a point-release (13.2 -> 13.3). I'd assume that one could've easily been deprecated first with a message saying how to get rid of it... And same thing: breaking changes should really only happen in major releases ;).

I'm maintaining someone else code here, which had:

[<AutoOpen>]
module FsHttpExtensions =
    let makeCustUrl suffix = $"{HttpConfig.BaseUrl}/customers/{DataSetup.KNOWN_CUSTOMER_GUID}/%s{suffix}"
    let makeBaseUrl suffix = $"{HttpConfig.BaseUrl}/%s{suffix}"

    type IRequestContext<'T> with

        /// Create a GET request from HttpConfig.BaseUrl
        [<CustomOperation("GET_BASE")>]
        member this.GetBase(context: IRequestContext<StartingContext>, suffix) = this.Get(context, makeBaseUrl suffix)

        /// Create a POST request from HttpConfig.BaseUrl
        [<CustomOperation("POST_BASE")>]
        member this.PostBase(context: IRequestContext<StartingContext>, suffix) = this.Post(context, makeBaseUrl suffix)

Here I just replaced the StartingContext with _ for auto-generalization. It now defaults to HeaderContext. Not sure that is correct. I hope the code will still run.

I can always just rollback and go with an older version, but I prefer not to.

image

SchlenkR commented 2 months ago

Thanks @abelbraaksma for the points, and time taken. I see no ranting. I appreciate that you shared your thoughts.

To make a long story short: There's a gap to that has to be bridged; between what is realistic for me as maintainer, and the understandable and legitim points brought up here (and other points).

Regarding StartingContext: It disappeared for the sake of being able to specify template-like requests (using the CE notation), and then using the CE notation to define the verb later (see here: https://fsprojects.github.io/FsHttp/Composability.html) Before that change, StartingContext was used to constrain that the verb must come before any request headers or body. But there's this comment that I might be come up one day: https://github.com/fsprojects/FsHttp/blob/625184c53278b704daae713adb36d0c8a2863988/src/FsHttp/Dsl.fs#L25-L27

Apart from that, I (currently) don't see any big changes coming up, and I regard the library and it's API as "quire stable" :)

Thank you, and again, I appreciate your thouhgs very much.

abelbraaksma commented 1 month ago

Hi @SchlenkR, thanks for the insights and pointers!

About "keeping two eyes on it", quite recently, a new build task was added for .NET projects which allows you to check for binary incompatibilities of any .NET library against a baseline version in NuGet.

From what I read there, it is a very small change to your fsproj files and it will, at the very least, throw an error if this happens accidentally.

abelbraaksma commented 1 month ago

PS: I believe this can be closed with no action, as apart from me documenting this behavior by reporting it, there's not really anything to change r.n. ;).

SchlenkR commented 1 month ago

Reopening the issue with changed title.

Thanks @abelbraaksma for the info. That seems to be worth having a look at and eventually implement it.

About "keeping two eyes on it", quite recently, a new build task was added for .NET projects which allows you to check for binary incompatibilities of any .NET library against a baseline version in NuGet. ... From what I read there, it is a very small change to your fsproj files and it will, at the very least, throw an error if this happens accidentally.