Add netcoreapp build #1418
Add netcoreapp build #1418
Conversation
|
@ghuntley Can you take a look when you have a chance? |
olevett
left a comment
olevett
left a comment
There was a problem hiding this comment.
Thanks for this 👍
Couple of questions/thoughts but nothing major!
| { | ||
| public void Register(Action<Func<object>, Type> registerFunction) | ||
| { | ||
| RxApp.TaskpoolScheduler = TaskPoolScheduler.Default; |
There was a problem hiding this comment.
I'm a little wary of this for a couple of reasons
- I think TaskpoolScheduler should already be configured on non-portable platforms (which covers netcoreapp1.0) e.g. https://github.com/reactiveui/ReactiveUI/blob/develop/src/ReactiveUI/RxApp.cs#L38
- I think the MainThreadScheduler should also get set a default value (I think DefaultScheduler.Instance) if nothing else sets it and I think setting TaskpoolScheduler but not MainThreadScheduler is potentially a bit confusing.
I'd be interested to know if there are problems if this line is omitted, or if it's fine without. I think I'd keep the PlatformRegistrations class however as it adds some value on it's own.
I also think it might be worth wiring up an IPlatformOperations even if it just returns null, as I think there are cases it would cause a warning to be logged otherwise (@ghuntley may disagree though...)
There was a problem hiding this comment.
- It looks like it is. Should I remove that setting from all of the platforms?
- I'll set it to
DefaultScheduler.Instance
The only place that logs a warning about a missing IPlatformOperations looks to be in RoutedViewHost, which is only available in builds using the Platforms/windows-common files.
There was a problem hiding this comment.
I think don't worry about removing it everywhere for now, I think separate issue (smaller scope = nicer to review :))
Don't worry about the PlatformOperations then, does look like we shouldn't be hitting it at all :)
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' "> | ||
| <Compile Include="Platforms\netcoreapp1.0\**\*.cs" /> |
There was a problem hiding this comment.
Related to this, I'd add an additional PropertyGroup to Directory.build.targets for netcoreapp. I think it might make sense to define the NETFX_CORE constant there too, but can't remember what exactly that includes off the top of my head. Will double check when I get the chance.
There was a problem hiding this comment.
I'll add the PropertyGroup, but I can't add the NETFX_CORE define since that define currently backs all usings of the Windows.UI.* namespaces.
There was a problem hiding this comment.
That seems fine then, might be we want to make NETFX_CORE two separate defines in future, but don't think it should be in scope for this change.
3 participants
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Support for a .NET Core non-crashing build.
What is the current behavior? (You can also link to an open issue here)
Any .NET Core usage of ReactiveUI will crash.
What is the new behavior (if this is a feature change)?
A .NET Core build will not crash upon load. The user must supply a scheduler for the
RxApp.MainThreadSchedulerdepending on their platform.What might this PR break?
Nothing.
Please check if the PR fulfills these requirements
Other information:
Fixes #1417