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

Migrate to netstandard2.0 #17

Merged
merged 6 commits into from
Oct 2, 2018
Merged

Migrate to netstandard2.0 #17

merged 6 commits into from
Oct 2, 2018

Conversation

vhatsura
Copy link
Contributor

@vhatsura vhatsura commented Sep 25, 2018

For #16
Some tricky cases:

  1. RazorEngine isn't compatible with netstandard. Replate with RazorLight nuget package
  2. GraphRunner has async interface due to async-only interface in RazorLight engine. It's a breaking change. For backward compatibility, was added non-async methods, which are called async methods with GetAwaiter().GetResult(). Such methods can be marked as Obsolete

Copy link
Member

@wozzo wozzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking on my phone at the moment and mostly looks good but will do a proper look soon. Couple of things for the meantime. Theres one of the cake generated scripts been added could that be removed - 645a1e2b-a9f4-459e-8d5e-a517e430b274.cake
There's a couple of async methods which could have the async and awaits removed. If the result of an awaited method is being immediately returned then the task itself can be returned - ParseAndCacheRazorTemplateAsync for example. Hope that makes sense. Oh amd just an fyi if you're writing library code like this then 99% of the time you should add .ConfigureAwait(false) when you do need to await. I'll see if I can look out a link which explains why but it's basically to allow the method to resume in a different thread context which is more efficient.
Thanks again for looking at this

@vhatsura
Copy link
Contributor Author

vhatsura commented Sep 26, 2018

Thank for fast response.
Regarding ConfigureAwait. ConfigureAwait(false) should be specified to avoid deadlocks, which can be occured in UI and ASP.Net (not ASP.Net Core) application. JFYI, Synchronization context was removed from ASP.Net Core (http://blog.stephencleary.com/2017/03/aspnetcore-synchronization-context.html).
So, as I understand Cake.Graph can be used in Cake related projects, which is run under Console Application. In this case, I think, it doesn't make any sense to specify ConfigureAwait(false)

@wozzo
Copy link
Member

wozzo commented Sep 26, 2018 via email

@wozzo
Copy link
Member

wozzo commented Oct 2, 2018

Cool. Looks good. Thanks for your contribution

@wozzo wozzo merged commit 1aca1aa into cake-contrib:develop Oct 2, 2018
wozzo added a commit that referenced this pull request Oct 2, 2018
* Migrate to netstandard2.0

* Add non-async methods to backward compatibility

Update Cake to 0.28.0 version for build

* Update Cake.FileHelpers to 3.1.0

* Fix nuget package generation

* Delete temporary file

* Remove redundant code
wozzo added a commit that referenced this pull request Oct 3, 2018
* Migrate to netstandard2.0 (#17)

* Migrate to netstandard2.0

* Add non-async methods to backward compatibility

Update Cake to 0.28.0 version for build

* Update Cake.FileHelpers to 3.1.0

* Fix nuget package generation

* Delete temporary file

* Remove redundant code

* Build with Cake 0.30.0 (#19)
wozzo added a commit that referenced this pull request Oct 7, 2018
* Migrate to netstandard2.0 (#17)

* Migrate to netstandard2.0

* Add non-async methods to backward compatibility

Update Cake to 0.28.0 version for build

* Update Cake.FileHelpers to 3.1.0

* Fix nuget package generation

* Delete temporary file

* Remove redundant code

* Build with Cake 0.30.0 (#19)

* Add RazorLight dependency #21 (#22)

* Add Example usage (#23)
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

Successfully merging this pull request may close these issues.

None yet

2 participants