SchlenkR/FsHttp

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

Opened this issue · 5 comments

abelbraaksma commented

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: fsprojects/FSharp.Control.TaskSeq#179, fsprojects/FSharp.Control.TaskSeq#167 and fsprojects/FSharp.Control.TaskSeq#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

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

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).

  • Marking things obsolete won't be possible for several reasons.
  • Breaking changes should definitely result in a major version, and must not be introduced in minor versions. That did happen; my honest apologies for the inconvenience. I will keep 2 eyes on it.
  • There is no migration log anymore. Instead, breaking changes (and all other changes) are going to be documented in the release notes located in build props, with some eventual hints on how to migrate (see here: https://fsprojects.github.io/FsHttp/Release_Notes.html)

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

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

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

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.

  • 👍1