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

Consider reviewing dependencies? #382

Closed
isaacabraham opened this issue Oct 19, 2019 · 13 comments
Closed

Consider reviewing dependencies? #382

isaacabraham opened this issue Oct 19, 2019 · 13 comments

Comments

@isaacabraham
Copy link
Contributor

Just a question if it's worth reviewing the dependency chain on Giraffe - I wonder if a few of packages in there are still needed (could be, I don't know):

  • FSharp.Core (>= 4.7)
  • Microsoft.AspNetCore.Authentication (>= 2.2)
  • Microsoft.AspNetCore.Authorization (>= 2.2)
  • Microsoft.AspNetCore.Diagnostics (>= 2.2)
  • Microsoft.AspNetCore.Hosting.Abstractions (>= 2.2)
  • Microsoft.AspNetCore.ResponseCaching (>= 2.2)
  • Microsoft.IO.RecyclableMemoryStream (>= 1.2.2)
  • Newtonsoft.Json (>= 12.0.2)
  • System.Text.RegularExpressions (>= 4.3.1)
  • System.ValueTuple (>= 4.5)
  • System.Xml.XmlSerializer (>= 4.3)
  • TaskBuilder.fs (>= 2.1)
  • Utf8Json (>= 1.3.7)

I'm particularly wondering about the bottom half e.g. ValueTuple, XmlSerializer, RegularExpressions etc.. - are these needed in the netstandard2+ world? They massively increase the size of e.g. paket graphs.

Again, it may be that they're all needed - but if they aren't, maybe they can be removed?

@slang25
Copy link
Member

slang25 commented Oct 20, 2019

Good point, here's my thoughts:

  • FSharp.Core (>= 4.7)
    • Yep, seems fair
  • Microsoft.AspNetCore.Authentication (>= 2.2)
    • Used in the Giraffe.Auth module
  • Microsoft.AspNetCore.Authorization (>= 2.2)
    • Used in the Giraffe.Auth module
  • Microsoft.AspNetCore.Diagnostics (>= 2.2)
    • Can go I think, not used
  • Microsoft.AspNetCore.Hosting.Abstractions (>= 2.2)
    • Can go I think
  • Microsoft.AspNetCore.ResponseCaching (>= 2.2)
    • Used in Giraffe.ResponseCaching module
  • Microsoft.IO.RecyclableMemoryStream (>= 1.2.2)
    • Recently added for serialization perf, could use it in more places. The 1.3.0 version released 2 days ago has newer TFMs added (this had been bugging me)
  • Newtonsoft.Json (>= 12.0.2)
    • Used, we have multiple serialization libraries, and I intend to add another (System.Text.Json, we get this "for free" in netcoreapp3.0 but must reference a package for earlier TFMs)
  • System.Text.RegularExpressions (>= 4.3.1)
    • Implicitly included as part of netstandard2.0 and reference assemblies in net461
  • System.ValueTuple (>= 4.5)
    • Same as above (additionally included via Utf8Json)
  • System.Xml.XmlSerializer (>= 4.3)
    • Same as above
  • TaskBuilder.fs (>= 2.1)
    • As you know we want netstandard2.0 added here. This could be included as source instead, which would sidestep another dependency. However users of Giraffe would need to go and add this package themselves if they were already using it in their code (quite common, also Saturn would require it)
  • Utf8Json (>= 1.3.7)
    • Required. It could make sense to move the different JSON serialization options to their own package

@ssiltanen
Copy link

Newtonsoft.Json (>= 12.0.2)
Used, we have multiple serialization libraries, and I intend to add another (System.Text.Json, we get this "for free" in netcoreapp3.0 but must reference a package for earlier TFMs)

When you say "we get this "for free"", is it already possible to use System.Text.Json with giraffe, or was it a reference to it not needing another external dependency?

@slang25
Copy link
Member

slang25 commented Oct 21, 2019

What I mean here is that we use the Microsoft.AspNetCore.App FrameworkReference here, so we don't need to add an additional dependency as it's already part of Microsoft.AspNetCore.App.

@isaacabraham
Copy link
Contributor Author

Just as an example, moving from netcore2 to netcore3, removing the dependencies on the System. packages above for netcore3 and promoting Taskbuilder to netstandard2.0 results in a drastically simpler paket lock file (319 lines -> 25 lines):

STORAGE: NONE
RESTRICTION: == netcoreapp3.0
NUGET
  remote: https://api.nuget.org/v3/index.json
    FSharp.Core (4.7)
    Microsoft.IO.RecyclableMemoryStream (1.3)
    Newtonsoft.Json (12.0.3)
    System.Reflection.Emit (4.6)
    System.Reflection.Emit.Lightweight (4.6)
    System.Threading.Tasks.Extensions (4.5.3)
    System.ValueTuple (4.5)
    Utf8Json (1.3.7)
      System.Reflection.Emit (>= 4.3)
      System.Reflection.Emit.Lightweight (>= 4.3)
      System.Threading.Tasks.Extensions (>= 4.4)
      System.ValueTuple (>= 4.4)
  remote: C:\Users\Isaac\Source\LocalPackages
    Giraffe (4.1.0)
      FSharp.Core (>= 4.7)
      Microsoft.IO.RecyclableMemoryStream (>= 1.2.2)
      Newtonsoft.Json (>= 12.0.2)
      TaskBuilder.fs (>= 2.1)
      Utf8Json (>= 1.3.7)
    TaskBuilder.fs (2.2.0)
      FSharp.Core (>= 4.1.17)

Combined with the recent perf enhancements to Paket that @forki has done, we can do a full paket install / update in 3 seconds instead of 11 seconds.

@NinoFloris
Copy link
Member

If we're going to change some of these, we should also include an explicit FSharp.Core reference so net461 and netstandard2.0 users on netcoreapp2.x don't get downgrade warnings.

Giraffe 4.0 is referencing >= 4.7.0 through the 3.x sdk but there's no reason it can't work with >= 4.6.2.

@forki
Copy link
Contributor

forki commented Dec 16, 2019

where do you get 4.6.2 from?

@NinoFloris
Copy link
Member

NinoFloris commented Dec 16, 2019

4.6.2 is the default FSharp.Core reference for < 3.0 sdks (at their latest patches)

@dustinmoris
Copy link
Member

Is support for net461 even still required?

@dustinmoris
Copy link
Member

@isaacabraham Am I right to assume that the 5.0.0-xxx packages have resolved this issue?

@slang25
Copy link
Member

slang25 commented Jun 20, 2020

We have an issue with Taskbuilder.fs still, but that would go with #421

@semyon2105
Copy link
Contributor

There are multiple JSON dependencies still. It would be nicer if Newtonsoft.Json and Utf8Json didn't get pulled by default and came as separate integration packages. The view engine should also be separated out IMO

@baronfel
Copy link
Contributor

The view engine should also be separated out IMO

This has actually already been done in the new prereleases, so that's one off your list :)

@dustinmoris
Copy link
Member

Currently all dependencies are required, but we could reduce them by separating out the JSON serializers.

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

8 participants