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

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

Open
abelbraaksma opened this issue Apr 29, 2024 · 5 comments

Comments

@abelbraaksma
Copy link

abelbraaksma commented Apr 29, 2024

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

abelbraaksma commented Apr 29, 2024

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

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

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

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 SchlenkR reopened this May 9, 2024
@SchlenkR SchlenkR changed the title Undocumented backward incompatible parameter change in functions like deserializeJsonWithTAsync and toJsonTAsync etc Guard semantic versioning for major changes using binary compat checks in .fsproj May 9, 2024
@SchlenkR
Copy link
Owner

SchlenkR commented May 9, 2024

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.

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

2 participants