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

Separate client/server remote options #165

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

twsouthwick
Copy link
Member

@twsouthwick twsouthwick commented Aug 24, 2022

Fixes #112

@twsouthwick
Copy link
Member Author

@mjrousos I believe your PR #169 will have merge conflicts here :)

@CZEMacLeod
Copy link
Contributor

I'm a little confused with this. I hope you don't mind me asking, but the client/server nomenclature is odd here, given that it is 2 servers, one proxied behind the other. I understand having options which only have relevant properties for each server, Framework and Core, but I would have kept a base object with the properties that are common to both - such as ApiKey and it would probably mean that #169 wouldn't have merge issues.
Perhaps RemoteAppCoreOptions and RemoteAppFrameworkOptions would make more sense, and just keep RemoteAppOptions but make it abstract?

@mjrousos
Copy link
Member

Yea, the naming here is kind of confusing. As @CZEMacLeod points out, these are both server apps. The rationale for the client/server naming here is that from the perspective of communication between the ASP.NET and ASP.NET Core apps (for proxying, remote auth, and remote session sharing), the ASP.NET Core app functions as a client sending requests to the ASP.NET app which is more like a server responding to requests.

We actually decided to not use client and server in the package names when we re-layered into Microsoft.AspNetCore.SystemWebAdapters, Microsoft.AspNetCore.SystemWebAdapters.CoreServices, and Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices to avoid this exact confusion. Based on the conversations we had on API design, though, it sounds like folks were ok with client/server names within those packages because the user wouldn't have any trouble knowing what to use since there'd be only one API available. I'm open to trying to standardize on Core/Framework instead of Client/Server, but honestly none of the names seem great to me. Another option would be to just drop the Client/Server distinction in names entirely now that the APIs are in separate packages, but I do think there's value in making it clear the APIs are different since they're likely to show up near one another in docs/samples/etc. despite coming from different packages.

Copy link
Member

@mjrousos mjrousos left a comment

Choose a reason for hiding this comment

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

If we can leave a shared remote options type that the client/server-specific ones each derive from, it will make other ongoing work like adding option validation easier. Other than that, this looks good to me.

@HaoK
Copy link
Member

HaoK commented Aug 25, 2022

I sort of like RemoteAppFrameworkOptions vs RemoateAppCoreOptions as opposed to Client/Server personally

@twsouthwick
Copy link
Member Author

@HaoK let's discuss that in the API review. We had tentatively come up with client/server and this is the final change to enable that. If we want to change to framework/core it'll be easier to do after this PR across the whole code base as it'll be pretty much an exercise in find/replace.

@twsouthwick twsouthwick merged commit 37c9be3 into main Aug 26, 2022
@twsouthwick twsouthwick deleted the tasou/separate-client-server-remote-options branch August 26, 2022 00:08
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.

Feedback from initial API Review
4 participants