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

TaskBuilder update breaks my tasks #15

Closed
forki opened this issue Feb 19, 2018 · 30 comments
Closed

TaskBuilder update breaks my tasks #15

forki opened this issue Feb 19, 2018 · 30 comments

Comments

@forki
Copy link

forki commented Feb 19, 2018

  • TaskBuilder.fs (1.0.1)
  • TaskBuilder.fs (1.1)

image

any ideas? It breaks everything for me

@gusty
Copy link
Contributor

gusty commented Feb 19, 2018

Is this after applying my PR #14 ?

@forki
Copy link
Author

forki commented Feb 19, 2018 via email

@gusty
Copy link
Contributor

gusty commented Feb 19, 2018

Ah ok, since you posted in the other issue I though it was related to my fix.
Anyway I can have a look, can you manage to get a minimal repro? If I find a solution we can add it as a test.

@forki
Copy link
Author

forki commented Feb 19, 2018 via email

@forki
Copy link
Author

forki commented Feb 19, 2018

let t = task { return Some "x" }

let getX  = task {
    let! r = t
    if r = None then
        return! failwithf "Could not find x" 
    else
        return r
}

@gusty
Copy link
Contributor

gusty commented Feb 19, 2018

I found a way to solve it in my PR but good luck with the original code ;)

If you want I can include another commit with this test and its fix.

@forki
Copy link
Author

forki commented Feb 19, 2018

yes please. it's a regression and hurts us badly.

@rspeele
Copy link
Owner

rspeele commented Feb 19, 2018

I will be checking out Gustavo's PR this week to get F# 4.0 compat back. Sounds like it should be possible to fix with that code.

In the meantime here is a quick fix for this regression applied to the existing codebase:

https://www.nuget.org/packages/TaskBuilder.fs/1.1.1

Just out of curiosity, is there any reason you use return! instead of return here?

@forki
Copy link
Author

forki commented Feb 19, 2018 via email

gusty added a commit to gusty/TaskBuilder.fs that referenced this issue Feb 19, 2018
@gusty
Copy link
Contributor

gusty commented Feb 19, 2018

I also think return makes more sense and am curious to hear @dsyme explanations.

Anyway I updated my PR with the fix. If you can pull it and test your code please let me know.

@forki
Copy link
Author

forki commented Feb 20, 2018 via email

@dsyme
Copy link

dsyme commented Feb 20, 2018

Just out of curiosity, is there any reason you use return! instead of return here?

You can do either - either way the failwith will fail to return either "return" type unit or Task<unit> or whatever.

@forki
Copy link
Author

forki commented Feb 20, 2018 via email

@rspeele
Copy link
Owner

rspeele commented Feb 26, 2018

Just FYI, 1.2.0 rc is available now.
This has Gustavo's changes for 4.0 compability. It should behave the same way both in terms of type inference and runtime performance, and seems to from my testing, but I don't have a big codebase to check against.

@forki would you be willing to try it out and see how it works with your code?

@forki
Copy link
Author

forki commented Feb 26, 2018

 The module/namespace 'FSharp.Control.Tasks.TaskBuilder' from compilation unit 'TaskBuilder.fs' did not contain the namespace, module or type 'ContextInsensitiveTaskBuilder'

@forki
Copy link
Author

forki commented Feb 26, 2018

so basically whole projet breaks again. what do I need to do? and can you please write that into release notes

@gusty
Copy link
Contributor

gusty commented Feb 26, 2018

Yes, this is a minor issue, easy to fix, there were some refactoring in the namespaces/modules.

Looking at the code the way it is right now you won't find ContextInsensitiveTaskBuilder anywhere, you will find that builder like this:

FSharp.Control.Tasks.ContextInsensitive.TaskBuilder

Now, there are 2 ways to fix it, either you fix your code, or we can have a look at the taksbuilder and see if we can fix the taskbuilder code.

But I wonder why are you referencing that builder class? The only thing you need to reference is the builder instance:

FSharp.Control.Tasks.ContextInsensitive.task

or ideally:

open FSharp.Control.Tasks.ContextInsensitive

let x = task { ...

@rspeele
Copy link
Owner

rspeele commented Feb 26, 2018

In the RC, there is no longer a separate class for ContextInsensitiveTaskBuilder. There's just one TaskBuilder type and different extension methods on that type depending which module you open.

In what context are you referring to the task builder type by name? You could "fix" it by referring to FSharp.Control.Tasks.TaskBuilder.TaskBuilder but right now that isn't a great option as that module is marked obsolete (just to hide it from intellisense while still being able to use it from inline code). We might have to refactor and add type aliases to keep code that referred to the type name ContextInsensitiveTaskBuilder working.

@forki
Copy link
Author

forki commented Feb 26, 2018

I'm lost. What namespace do I need to open to get old behavior?

@gusty
Copy link
Contributor

gusty commented Feb 26, 2018

@forki. See the last snippet in my previous comment.

@forki
Copy link
Author

forki commented Feb 26, 2018

image

I still get that error everywhere. Is it related to giraffe? Do they need to update first?

@gusty
Copy link
Contributor

gusty commented Feb 26, 2018

I'm not sure if this is your case but if you are using TaskBuilder.fs in your project and at the same time your project uses another project that uses TaskBuilder (like Giraffe) you might run into problems, since the namespaces are the same.

I think ideally libraries using TaskBuilder shouldn't re-export TaskBuilder to avoid this.

@forki
Copy link
Author

forki commented Feb 26, 2018

/cc @dustinmoris

@dustinmoris
Copy link

@gusty Could you elaborate what you mean? How can I not expose the task CE after referencing TaskBuilder.fs?

@gusty
Copy link
Contributor

gusty commented Feb 26, 2018

Honestly I don't know, I just said "ideally", but I know that in some scenarios this is problematic, it happened to me with other libraries.

To answer your question I need to know more details, specifically how it is included in Giraffe, I haven't look at it.

But if you are copying the code, you can simply change the namespaces or mark it as internal, this will allow the users of Giraffe to use their own taskbuilders at possible different versions.

In the readme it states:

This is public domain code. I encourage you to simply copy TaskBuilder.fs into your own project and use it as you see fit. It is not necessary to credit me or include any legal notice with your copy of the code.

I guess it's also fine to change the namespaces, but I know it's not an elegant solution.

@ForNeVeR
Copy link
Contributor

I think newer versions of Giraffe use TaskBuilder from NuGet, right?

https://github.com/giraffe-fsharp/Giraffe/blob/4b5852e80e49c11e11bb66a4065afd79b6ad778b/src/Giraffe/Giraffe.fsproj#L35

@forki
Copy link
Author

forki commented Feb 27, 2018 via email

@forki
Copy link
Author

forki commented Mar 6, 2018

@rspeele not sure why you closed it, but I can now confirm that a simultaneous update of taskbuilder and giraffe indeed works. so can you and @dustinmoris please somehow sync?

@dustinmoris
Copy link

Hi, just to reiterate everything:

  • In theory there were breaking changes between TaskBuilder.fs 1.0.1 and the versions after (1.1 and now 1.2-rc).
  • Giraffe has a dependency on the the TaskBuilder.fs NuGet package.
  • Updating Giraffe to the latest 1.2-rc build of TaskBuilder.fs fixes the errors for @forki because there is no conflict between the old and the new namespaces of TaskBuilder.fs.
  • However, a project which references Giraffe doesn't require an explicit reference to TaskBuilder.fs anymore, unless it wants to use a higher version and do some assembly redirects.

As @forki suggested if we coordinate the release of TaskBuilder.fs and Giraffe in the future then such breaking conflicts can be avoided for end users, but I also believe that perhaps it would have been better to version TaskBuilder.fs accordingly to the breaking changes (e.g. from 1.x.x to 2.x.x) and then it would have been more obvious (and expected) that an additional reference to a higher major version of TaskBuilder.fs might have caused issues with Giraffe if Giraffe hasn't been updated yet.

Am I missing something?

So my final question is what is the ETA for TaskBuilder.fs 1.2 stable?

@rspeele
Copy link
Owner

rspeele commented Mar 20, 2018

I opened a new issue for planning the release. #18

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

6 participants