Skip to content

Blazor WebAssembly support: added try-catch for unsupported properties #274

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

Merged
merged 6 commits into from
Sep 15, 2020

Conversation

bjorg
Copy link
Contributor

@bjorg bjorg commented Sep 6, 2020

Addresses remaining compatibility issues for Blazor WebAssembly support (see #262)

@sungam3r
Copy link
Member

sungam3r commented Sep 7, 2020

Just a thought. Could/should we exclude these properties from compilation for different platforms (multitargeting). Silently catching exceptions may be not optimal. Call to Debug.WriteLine will be excluded from release build.

@Shane32
Copy link
Member

Shane32 commented Sep 7, 2020

Just a thought. Could/should we exclude these properties from compilation for different platforms (multitargeting). Silently catching exceptions may be not optimal. Call to Debug.WriteLine will be excluded from release build.

I might not exclude them, as they are not excluded from the targeted platform, and it complicates the build process. Perhaps future versions of the platform will support it. But I agree that silently catching exceptions isn't optimal.

Here's another idea: perhaps only set these properties if they were set in the options class. Then, if a person was to trying to set one of these properties, they would property receive a PlatformNotSupported exception when attempting to execute the query. I've not looked at the code in-depth so maybe that doesn't work or isn't applicable.

@sungam3r
Copy link
Member

sungam3r commented Sep 7, 2020

This will work if the getters for these properties don't throw PNSE. Then you can compare the values and call setters only if values differ.

@rose-a
Copy link
Collaborator

rose-a commented Sep 8, 2020

Here's another idea: perhaps only set these properties if they were set in the options class.

I'd prefer this approach, too.

@bjorg
Copy link
Contributor Author

bjorg commented Sep 8, 2020

I'll investigate today what can be done. First, I'll check if the getter can be used. If so, I'll use a comparison to set it. Otherwise, I'll add a field to the options class to track when a property is set and key off that.

@bjorg
Copy link
Contributor Author

bjorg commented Sep 8, 2020

The getter fails on ((HttpClientHandler)Options.HttpMessageHandler).ClientCertificates (ditto for UseDefaultCredentials). So it's not even possible to read an option that was set by the user. That said, how would user ever set it in the first place? This makes me think the first approach of swallowing the exception may be the way to go. Thoguhts?

@Shane32
Copy link
Member

Shane32 commented Sep 8, 2020

What if UseDefaultCredentials was a nullable boolean and only sets the property if it HasValue? And similarly only set the other property if not null?

@bjorg
Copy link
Contributor Author

bjorg commented Sep 8, 2020

@Shane32 Unfortunately, ClientCertificates and UseDefaultCredentials are properties on the built-in System.Net.Http.HttpClientHandler class. Otherwise, I would have added a boolean field to track if they were modified.

@bjorg
Copy link
Contributor Author

bjorg commented Sep 8, 2020

Pushed a few more changes:

  1. Preserve query parameters in endpoint URI (this is required for authenticating with AppSync)
  2. Allow end-point URI to begin with wss:// or ws://. Unfortunately, AppSync uses a different hostname for their REST vs. WebSocket API endpoints. Being able to specify the wss:// endpoint directly will make life a bit easier.
  3. Automatically set Options.UseWebSocketForQueriesAndMutations to true when the endpoint scheme is either wss:// or ws://.

@sungam3r
Copy link
Member

sungam3r commented Sep 8, 2020

Let's not mix different fixes in one PR.

@sungam3r
Copy link
Member

sungam3r commented Sep 8, 2020

This makes me think the first approach of swallowing the exception may be the way to go.

Yes, possible. You just need to clearly reflect this issue in the documentation/faq.

@bjorg
Copy link
Contributor Author

bjorg commented Sep 8, 2020

Yes, possible. You just need to clearly reflect this issue in the documentation/faq.

Ok. Which doc do I need to amend?

@bjorg
Copy link
Contributor Author

bjorg commented Sep 8, 2020

Let's not mix different fixes in one PR.

Reverted. Will file new issue and submit new PR.

@Shane32
Copy link
Member

Shane32 commented Sep 8, 2020

@Shane32 Unfortunately, ClientCertificates and UseDefaultCredentials are properties on the built-in System.Net.Http.HttpClientHandler class. Otherwise, I would have added a boolean field to track if they were modified.

I see.

This makes me think the first approach of swallowing the exception may be the way to go.

Yes, possible. You just need to clearly reflect this issue in the documentation/faq.

Agreed

@sungam3r
Copy link
Member

sungam3r commented Sep 8, 2020

Ok. Which doc do I need to amend?

Readme.md

@bjorg
Copy link
Contributor Author

bjorg commented Sep 9, 2020

Added a note on Blazor WebAssembly limitations to ReadMe.md.

@Shane32
Copy link
Member

Shane32 commented Sep 9, 2020

The getter fails on ((HttpClientHandler)Options.HttpMessageHandler).ClientCertificates (ditto for UseDefaultCredentials). So it's not even possible to read an option that was set by the user. That said, how would user ever set it in the first place? This makes me think the first approach of swallowing the exception may be the way to go. Thoguhts?

I missed this earlier. Looks like you’ve got it all figured out! 👍

@bjorg
Copy link
Contributor Author

bjorg commented Sep 9, 2020

Let me know if anything else is needed to merge this change.

@sungam3r sungam3r requested a review from rose-a September 12, 2020 23:33
@rose-a rose-a merged commit ba2e143 into graphql-dotnet:master Sep 15, 2020
@rose-a
Copy link
Collaborator

rose-a commented Sep 15, 2020

Thanks @bjorg for your contribution! And thanks to @Shane32 and @sungam3r for supporting him ;)

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.

4 participants