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

Add warning about .net standard project #2164

Closed
glennawatson opened this issue Sep 9, 2019 · 19 comments · Fixed by #2186

Comments

@glennawatson
Copy link
Contributor

@glennawatson glennawatson commented Sep 9, 2019

Add warning message if using the .NET Standard platform.

It was previous happening before this version came in this file

We have to be careful so we don't confuse WASM users as well.

@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

Can give this a try if needed :)

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

@rgomez90 -- sure if you like. You can probably add a check in RxApp.cs -- if it's the DefaultScheduler it's likely to be wrong.

@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

@glennawatson with warning message do you mean #warning or LogHost.Default.Warn("...");?

I suppose the second, so if I understand correctly, I should just check the Scheduler in RxApp and if it's the DefaultScheduler I should call LogHost.Default.Warn("ReactiveUI needs a platform to run, it cannot run on .NET Standard");

But where should I do the check? In the static ctor or in the MainThreadScheduler setter?

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

I would maybe even put it in the MainThreadScheduler getter with a bool value you set to avoid it happening excessively.

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

and LogHost is the correct approach., #warning would be compile time only.

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

so

if (!_hasSchedulerBeenChecked && schedulerType == ...)
{
   _hasSchedulerBeenChecked = true;

Thanks for helping out with this one. If you haven't seen it sign up for a free tshirt here https://hacktoberfest.digitalocean.com

Hacktoberfest
Open source is changing the world – one pull request at a time.
@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

@glennawatson thank you for the quick replies!

Yes, I already signed (like previous years), but this is not the reason I am contributing.

I wrote some sort of auto-measurement and device management app for my company (we produce transducers) and ported it to use ReactiveUI.

Since them I am in love with it, and helped me a lot to introduce myself in reactive programming....without it I wouldn't have put my nose in there...

That's the reason, I will use A LOT ReactiveUI and also want to give something back, to this really beautiful library 😃 I learned (and still learn) a lot from it / from you core contributors!

@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

Something like this would be ok?

public static IScheduler MainThreadScheduler
{
    get
    {
        if (_unitTestMainThreadScheduler != null)
        {
            return _unitTestMainThreadScheduler;
        }

        // If Scheduler is DefaultScheduler, user is likely using .NET Standard
        if (!_hasSchedulerBeenChecked && _mainThreadScheduler == Scheduler.Default)
        {
            _hasSchedulerBeenChecked = true;
            LogHost.Default.Warn("ReactiveUi cannot run on .NET Standard! It needs a platform to run! (.Net Core, WPF, UWP...)");
        }

        return _mainThreadScheduler;
    }

    set
    {
      ...
    }
}
@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

It can run in .NET Standard 2.0 but in very limited scenarios.

So web assembly projects such as Uno/Blazor will run the .net standard project in the web browser but those project types should be setting their own main thread scheduler as well so it should never be default.

So I would just say something along the lines of you need to add a nuget host package, such as ReactiveUI.WPF, ReactiveUI.Blazor etc.

We tend to do the I as upper case in the UI, so ReactiveUI.

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

So what I mean is in the blazor/uno case, the user should install ReactiveUI.Blazor or ReactiveUI.Uno, which will change the main thread scheduler.

@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

@glennawatson thank you again for your explanation. I have close to no idea about the new Blazor and Uno. (Blazor is in my To-Do list)

So something like this should be better then

...
 if (!_hasSchedulerBeenChecked && _mainThreadScheduler == Scheduler.Default)
    {
        _hasSchedulerBeenChecked = true;
        LogHost.Default.Warn("It seems you are running .NET Standard, but there is no host package installed!\n");
        LogHost.Default.Warn("You will need to install the specific host package for your platform (ReactiveUI.WPF, ReactiveUI.Blazor, ...)\n");
        LogHost.Default.Warn("You can install the needed package via NuGet");
    }
...
@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

Seems good with the message.

Btw the .net standard mode only happens when you use Blazor client mode, they have a server mode that runs asp.net and signalr which is .NET Core 3 based.

rgomez90 added a commit to rgomez90/ReactiveUI that referenced this issue Oct 1, 2019
@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

@glennawatson was about to submit the PR, but have readed the Contribution Guidelines where is said, I should make a PR to develop branch....

But I can't find a develop branch, only a master & preview/11.0.0 should I make PR against master?

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

If you want you can make a pr after this one to mention pr against master.

@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

Me not understand...

I have forked the project and I created a branch in my fork fix2164 where I made the changes...

Now I want to make PR and I need to select a base repository and branch. In your Contribution Guidelines - Submitting a Pull Request is said In GitHub, send a pull request to reactiveui:develop, but there is no develop branch...

image

So which branch should I select as base branch? master?

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

I meant you can change the documentation via a PR to say master.

@rgomez90

This comment has been minimized.

Copy link
Contributor

@rgomez90 rgomez90 commented Oct 1, 2019

@glennawatson Oh now I understand, excuse me....it's getting late 😅

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

Sorry getting a bit tired here hence the hard to decipher comment

@glennawatson

This comment has been minimized.

Copy link
Contributor Author

@glennawatson glennawatson commented Oct 1, 2019

But the contribution guide is out of date hence it needs to be updated

rgomez90 added a commit to rgomez90/ReactiveUI that referenced this issue Oct 1, 2019
Update target base branch when submitting a pull request like spoken in reactiveui#2164 (reactiveui#2164 (comment))
RLittlesII added a commit that referenced this issue Oct 1, 2019
Update target base branch when submitting a pull request like spoken in #2164 (#2164 (comment))
RLittlesII added a commit that referenced this issue Oct 1, 2019
* Add warning when running .NET Standard

#2164

* Add reference to installation instructions

Co-Authored-By: Artyom <worldbeater-dev@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.