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 possibility for caller proxy to return an observable #330

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Jopie64
Copy link

@Jopie64 Jopie64 commented Feb 28, 2021

First part of #238.
Can you take a look at it for whether I'm on the good track with this feature?

@Jopie64
Copy link
Author

Jopie64 commented Feb 28, 2021

Also I was wondering which C# language features I could use... Is C# 7 or even 8 allowed?

@darkl
Copy link
Member

darkl commented Mar 1, 2021

C# 7.0 is okay. C# 8.0 is not supported for .NET Framework, so let's avoid it (I hope I have been doing that myself).

@Jopie64 Jopie64 force-pushed the master branch 3 times, most recently from 77226d9 to 2c91ef6 Compare March 2, 2021 15:49
When subscribed to it will call a progressive RPC

Code-Sharp#238
As discussed in the review, this extra declaration is a requirement to
make sure that people really want the method to be progressive and not
return a IObservable derived type by accident.

Code-Sharp#238
@Jopie64
Copy link
Author

Jopie64 commented Mar 5, 2021

Hi @darkl

I think I have most of it, if not everything, in now to support IObservable return values on the callee side as well as on the caller side. Can you please take another look at it and show me where I might have messed up? :)
Thanks in advance!

@darkl
Copy link
Member

darkl commented Mar 6, 2021

Thanks @Jopie64 for your work!
It might take me some time to review this.

@darkl
Copy link
Member

darkl commented May 2, 2021

I am sorry this is taking so long, I am busy with other stuff.

One thing that bothers me is that the final value will always be empty. This causes inconsistencies because if you have an old implementation using Task and IProgress and then you "forward" it using a callee that uses a callee proxy that returns a cold observable, then it will no longer return the return value of the original implementation. I'm wasn't able to find a solution that I'm happy with for this problem.

Elad

@Jopie64
Copy link
Author

Jopie64 commented May 2, 2021

Yea I feel your pain, there's always lots of important stuff to do for us devs. No hurries 😊

About that final value thing. That bothered me too for a while. But in practice it doesn't really matter at least for us. The thing to keep in mind is: It is not a 100% replacement for the IProgress API.

  • When you don't want to have the intermediate types be the same as the type of the final value, don't use this pattern.
  • When you want the final yield to contain a value, don't use this pattern.
  • Also when you want to use it for arbitrary API forwarding, where you don't know how the other side is using the progressive API, don't use this pattern.

In all other cases, where you just have an observable implementation on a different node than the observable subscription, this pattern can be used. For us in practice it means that we never had to make use of the extra capabilities the IProgress interface gives us. (We are currently using a workaround to be able to use this pattern.)

@darkl
Copy link
Member

darkl commented May 2, 2021

I think I want to change the behavior so that the CalleeProxy will omit the return value unless specified explicitly (maybe through an attribute or something).
Otherwise the behavior is too confusing at least to me.

Elad

@Jopie64
Copy link
Author

Jopie64 commented May 3, 2021

I don't understand what you mean... I assume you mean the final return value? In case of observables the callee already omits its final return value right? Because OnCompleted() doesn't carry a value...

It's the caller that you might consider confusing I think. In WAMP the final return might carry a value or it might not. In case of observables you don't expect the final result to contain a value. But I currently made it so that when it does anyway, it will emit this before it completes. The reason I did it this way is that this way the caller can use observable return values in all methods, where the server can implement them as observable or task.

In practice this works well for us. The caller (even when it is mostly javascript) uses observables everywhere and the server backend implements it however it wants. And an existing API can even switch from task to observable without requirering a change in the frontend.

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.

2 participants