search

giraffe-fsharp / giraffe-template

A dotnet new template for Giraffe web applications.
Apache License 2.0
37 stars 22 forks source link

Add -EnablePaket flag #1

Closed dustinmoris closed 6 years ago

dustinmoris commented 6 years ago

Moving the original issue from the giraffe-fsharp/Giraffe repo over here.

Original issue by @nicolocodev:

I'm working on the template project to add the flag -enablepaket 'cause paket is nice 💪

I'm using custom operations in order to use conditionals on .fsproj:

<!-- if(enablepaket) -->
  <Import Project="..\..\.paket\Paket.Restore.targets" />
  <!-- else -->
  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Cors" Version="2.0.*" />

it's not working by now, always get the conditional comments on fsproj... but what do you think 👍 👎?

dustinmoris commented 6 years ago

PR approved, if someone wants to help with this issue then please submit a PR!

dburriss commented 6 years ago

I have done a fair bit of work with conditionals on templates. Will take a look.

dustinmoris commented 6 years ago

Awesome, thanks!

dustinmoris commented 6 years ago

May I only suggest that if any conditional logic in an .fsproj file would prevent the project to compile normally from an IDE then I'd probably favour a copy/replace of a second .fsproj rather than if/else statements in the project file, so that maintenance of the code remains rather easy.

dburriss commented 6 years ago

@dustinmoris do you mean the generated .fsproj or the template .fsproj?

dustinmoris commented 6 years ago

The template fsproj...

dburriss commented 6 years ago

Ok so my son eventually took a nap so got to work on this :)

A few notes:

dburriss commented 6 years ago

@dustinmoris are you happy to leave out the paket binary from the template and leave that to the user to acquire?

dustinmoris commented 6 years ago

--UsePaket

Like it!

... and instead added it to run the 1st time build.bat is run

Like it too :)

We could just make it the users problem to make sure Paket is installed?

I don't think I follow entirely what the problem is. Ideally a user should only have to run the build script after the dotnet new command to get everything green. Are you saying that because of xplat this is not easily possible?

dburriss commented 6 years ago

@dustinmoris I have it working on Windows and Ubuntu 14.04 LTS I think there is a bug on Ubuntu though. The rename does not work so folders are AppNamePlaceholder instead of project name. I will verify and report on the templating repository if it is not already known.

I included the paket.bootstrapper.exe renamed to paket.exe. Seems less error prone than downloading it in the bat or sh file. What do you think? It is only 64KB.

dburriss commented 6 years ago

@dustinmoris WRT removing the version numbers as @forki suggests.

PROS

CONS

I have very little experience maintaining something with a large number of users but my gut says favor predictability because this is something that can create new instances at any time when the state of the surrounding environment (package versions on nuget) constantly change in ways outside of our control.

forki commented 6 years ago

The versions are still locked. Ship the lock file. That's the whole point. Everything will be fine.

Am 25.12.2017 07:14 schrieb "Devon Burriss" notifications@github.com:

@dustinmoris https://github.com/dustinmoris WRT removing the version numbers as @forki https://github.com/forki suggests.

PROS

  • The template uses the the Paket best practices
  • Less maintenance required for the template

CONS

  • Discrepancy between what packages could get pulled down at exact same time using one or the other
  • Following on from that there is a chance of the template not working at certain points without the user going and updating the versions. For example when AspnetCore went to version 2 but Giraffe did not yet support it would have been an instance when locking the versions would be needed.

I have very little experience maintaining something with a large number of users but my gut says favor predictability because this is something that can create new instances at any time when the state of the surrounding environment (package versions on nuget) constantly change in ways outside of our control.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/giraffe-fsharp/giraffe-template/issues/1#issuecomment-353835398, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNELH-GHAgmuRr3hopEqGkF3xM0j1ks5tDz1DgaJpZM4RK947 .

dustinmoris commented 6 years ago

Thanks for both of your help! Ok so I read everything on the PR as well and I believe that what @forki says if we include the paket.lock file with the fixed versions then Paket will always restore to these versions when a user installs the Giraffe template UNLESS one of the locked versions doesn't exist anymore (maybe package got deleted or unlisted, etc.) and then paket will use some logic to resolve to the next best match.

That sounds great to me and what I would want.

Am I right?

forki commented 6 years ago

It will always restore the locked version even when they are unlisted (which is a good thing). Only deletion is an issue that you can't solve. But then you get a descriptive error. It does not automatically just use something else. It always uses the lock file or reports an error.

Am 25.12.2017 10:50 schrieb "Dustin Moris Gorski" <notifications@github.com

:

Thanks for both of your help! Ok so I read everything on the PR as well and I believe that what @forki https://github.com/forki says if we include the paket.lock file with the fixed versions then Paket will always restore to these versions when a user installs the Giraffe template UNLESS one of the locked versions doesn't exist anymore (maybe package got deleted or unlisted, etc.) and then paket will use some logic to resolve to the next best match.

Am I right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/giraffe-fsharp/giraffe-template/issues/1#issuecomment-353855654, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNKdbWaE23bZr16DRb9IZ4ctV2OCCks5tD2_XgaJpZM4RK947 .

forki commented 6 years ago

One remark regarding unlisting: if you run paket install or paket update then it will not consider unlisted packages. But for restore it always restores what you committed. That's very important. The fact that nuget.exe behaves different is a long standing bug and reported long time ago

dburriss commented 6 years ago

@dustinmoris I started some work on automating the creation and updating of the paket.lock file in the template as this direction does start introducing quite a lot of friction. I am stopping that and just doing it manually for all permutations now so we can see how it goes. Once we are sure this is the way to go I will automate it.

PR is ready.

forki commented 6 years ago

What do you mean by automatic the creation of paket lock? You would just run install in the template folder and commit exactly that lock file. Please don't any magic around that.

Am 26.12.2017 11:02 schrieb "Devon Burriss" notifications@github.com:

@dustinmoris https://github.com/dustinmoris I started some work on automating the creation and updating of the paket.lock file in the template as this direction does start introducing quite a lot of friction. I am stopping that and just doing it manually for all permutations now so we can see how it goes. Once we are sure this is the way to go I will automate it.

PR is ready.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/giraffe-fsharp/giraffe-template/issues/1#issuecomment-353949160, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNLnAbip2XGUu8xNFlF3nDiUa9HUxks5tEMQZgaJpZM4RK947 .

dburriss commented 6 years ago

@forki Yes that was the 1st thing I tried. A few things that are a consequence of being a template rather than a normal fs project.

  1. "just run install" is required for Giraffe, DotLiquid, and Razor each time someone wants to update packages, even if they have made zero changes related to Paket. And of course any future author needs to know this. As things like WebSocket and who know what else get added this list will grow. So a simple script now to update the lock files from a single command would be appreciated by future me and others I am sure.
  2. "just run install" makes magic undesired changes to the ,fsproj file removing Condition="'$(UsePaket)' from the Import element of Project="..\..\.paket\Paket.Restore.targets"

If you know how I can solve point 2, point 1 becomes trivial. It would be great if we could keep it simple so any tips on how to fix the unintended re-write of the fsproj would be appreciated. You can find the offending line that gets re-written here: https://github.com/dburriss/giraffe-template/blob/develop/src/content/Giraffe/src/AppNamePlaceholder/AppNamePlaceholder.fsproj#L35

@dustinmoris as I mentioned what we are discussing now is a development improvement and the changes requested by yourself and forki are completed.

dustinmoris commented 6 years ago

@dburriss Thank you so much for the efforts. What you say makes sense and the maintenance of these files is certainly a big concern so thank you for looking into a future proof way as part of this PR as well!

Given that the lock file needs to be auto generated for each permutation (Giraffe without view engine, Giraffe without view engine and tests, Giraffe with Razor, Giraffe with Razor and tests, etc.) would it be not possible to just not include the lock file and instead pin the versions in the paket.references again? What is the problem of doing that @forki?

dburriss commented 6 years ago

@dustinmoris even if there isn't a way to stop Paket from undoing the fsproj conditional, I can generate the lock files outside and copy them in. This would share a bunch of code with the permutation tests which I would prefer to do in a separate PR.

dustinmoris commented 6 years ago

Ok, does it mean you want me to merge the current work as is and then wait for a follow up PR re dev improvement?

forki commented 6 years ago

If you pin the versions it basically becomes unusable for practical purposes. It's just not something paket users would appreciate. Tbh I don't really understand why you would want to support both. Either choose paket (like most F# users would do) or let paket users convert themselves. Most are already used to that. But shipping something that is not in the spirit of paket just makes not a lot of sense

Am 26.12.2017 23:29 schrieb "Dustin Moris Gorski" <notifications@github.com

:

Ok, does it mean you want me to merge the current work as is and then wait for a follow up PR re dev improvement?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/giraffe-fsharp/giraffe-template/issues/1#issuecomment-354020086, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNKRGaZf_pHPu5OgIckGfn5CFBO9Hks5tEXNGgaJpZM4RK947 .

dburriss commented 6 years ago

@dustinmoris Yes let's merge this with the lock files. As @forki says this is best practice (and thanks to @forki for his expertise here). We shouldn't sacrifice good practices for ease of maintenance on the template since it is fairly clear how we can mitigate the debt. If we couldn't mitigate the debt I think it would be best to leave this feature out. I will submit the automation of the tests and a script to update just the lock files in a PR linked to the permutation tests if that is ok?

dustinmoris commented 6 years ago

But shipping something that is not in the spirit of paket just makes not a lot of sense

Agreed. Thank you for the clarification. I really would love to ship the paket feature and make it a great experience.

Tbh I don't really understand why you would want to support both. Either choose paket (like most F# users would do) or let paket users convert themselves.

Yes I could just do that, but I really like the idea of making it easy and Paket friendly. I think it would massively increase the experience for NuGet and Paket users if they don't have to manually convert everytime they start using the template. In my opinion the success of a template is directly linked to how usable it is. If a user needs to quickly create a new project then the most convenient way should be the tepmlate regardless of which package manager they use.

dustinmoris commented 6 years ago

Published to NuGet now! Thanks again!

eugene-g commented 6 years ago

Shouldn't it be the default behavior? I'd rather have optional --no-paket flag.

forki commented 6 years ago

;-)

TheAngryByrd commented 6 years ago

I'd be in favor of it being default.

dustinmoris commented 6 years ago

Shouldn't it be the default behavior? I'd rather have optional --no-paket flag.

The question is should it not be the default by which standards?

After all Giraffe is a middleware sitting on top of the larger ASP.NET Core framework. All default ASP.NET Core templates use the built in NuGet client which comes by default with .NET Core SDK. I think from ASP.NET Core's point of view NuGet is probably considered the default.

From a developers point of view it might look different. So let's assume I'd want to create a new .NET Core web application. First I'd have to install the .NET Core SDK which (includes the NuGet client) and then... actually nothing else. No additional moving parts are needed. With the .NET Core SDK I am able to restore packages and build and run any .NET Core app. As far as documentation goes most Microsoft and OSS projects also use the .fsproj/.csproj files to add new dependencies, which means they all assume NuGet to be the default as well.

So from developers point of view there are not only less moving parts with NuGet, but also more online documentation matching the NuGet client case.

Tooling is a third aspect to consider. As far as I know there is good Paket support in all major .NET tools, however unlike NuGet they (mostly) require an additional extension to be installed by the developer first. This means there is yet again another moving part added before dependencies can be managed effectively in a project.

At last we could look at the F# community itself. I had a look at this month's most popular GitHub repositories in the F# language domain. By browsing the documentation of some of the most popular NuGet libraries I can see that most F# projects primarily refer to Paket when it comes to installing the package, which is probably an indicator that Paket is considered the default in the F# community.

Now with all this data it is certainly not clear which one to make the default in our case. Unfortunately Microsoft hasn't perfectly abstracted the responsibility of a package manager from the .NET Core SDK and has created a deep integration with NuGet which sort of puts other package managers a little bit into the shadow. I personally don't like it and would wish they would have made it more open to other package managers by design (maybe a little bit like they are more open to a variety of unit test runners), but they haven't. With that in mind and that I personally don't want to go against the default development experience I think I'd prefer to stick with the way it is now.

However, all of this is really only a minor point since the template has full Paket support now and anyone who wishes to use Paket or NuGet has all options available, which is the important thing to me.

dustinmoris commented 6 years ago

I highlighted the usage of Paket more prominently at the top of the README which explains how to create a new Giraffe application with the dotnet new command now: https://github.com/giraffe-fsharp/giraffe-template/commit/17734eb8db530dae4bcd28ceec8ac56098948765

image

eugene-g commented 6 years ago

@dustinmoris, thank you for detailed answer.

I understand your hesitation. On one side, making Paket default option improves its adoption. On another side, sticking to NuGet improves adoption of Giraffe. Don't we want both?

Leaving everything "as is" is fine, but we also have following options:

"By which standards?" question is open, and we're open community. Didn't we all struggle long enough without Paket to help other developers avoid the tar pit? Paket is the standard in F# community, because we care. We want to create better standards and improve quality of life for all the devs. If we were following "standards", Paket wouldn't exist in the first place.