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

Update to Blazor 3.0.0-preview6 #35

Merged
merged 2 commits into from
Jul 3, 2019

Conversation

theprateik
Copy link
Contributor

  • Update global.json to reflect the updated sdk version
  • Replace Json.Deserialize by JsonSerializer.Parse in class HubConnection
  • Add this. where missing in class HubConnection
  • Replace onclick and bind attributes in test razor pages by @OnClick and @Bind
  • Replace @functions by @code in test razor files
  • Remove UseSignalR and UseBlazor from test Server Startup class Configure method
  • Add UseAuthorization to Startup class
  • Comment out AddMessagePackProtocol and uncommented AddJsonProtocol in Startup class
  • Comment out AddMessagePackProtocol in ChatComponent class

- Update global.json to reflect the updated sdk version
- Replace Json.Deserialize by JsonSerializer.Parse in class HubConnection
- Add this. where missing in class HubConnection
- Replace onclick and bind attributes in test razor pages by @OnClick and @Bind
- Replace @functions by @code in test razor files
- Remove UseSignalR and UseBlazor from test Server Startup class Configure method
- Add UseAuthorization to Startup class
- Comment out AddMessagePackProtocol and uncommented AddJsonProtocol in Startup class
- Comment out AddMessagePackProtocol in ChatComponent class
@theprateik
Copy link
Contributor Author

#32

@theprateik
Copy link
Contributor Author

Not sure why the check is failing. But there is one issue though, that i noticed.
The test application throws error for Byte Array Argument request when using MessagePackProtocol. With JsonProtocol, this is not an issue.

@djinnet
Copy link

djinnet commented Jun 22, 2019

I found out that system.text.json don't support some certain datatypes yet as far I have been researched. I think bytes array is one of these datatypes that system.text.json doesn't fully support without converting the datatype into a string which is json, however, that will fail on the JsonProtocol.

I have tried to clone the project and tried to make the same setup except that I am using newtonsoft.json instead of system.text.json because it does support many datatypes.

I am using JsonConvert.DeserializeObject instead of jsonserializer.parse.

worth mentioning:
Bytes Array argument request is working on MessagePackProtocol and JsonProtocol on the newtonsoft.json version.

however, I am not sure if it is a bad idea to use newtonsoft.json instead of system.text.json and I hope that that you can give your thoughts on this. What do you think?

@galvesribeiro
Copy link
Member

Hello!

Could you please fix the build issue? Its been a while and the build doesn't have the history anymore so I can't check the problem.

@theprateik
Copy link
Contributor Author

There is no issue in building this locally.
I am not familiar with the DevOps and CI.
But last time, the reason it failed in DevOps was that the DevOps was not using .NET Core Preview 6 SDK.

@galvesribeiro
Copy link
Member

You need to change this and this to the appropriate SDK version.

Thanks!

@galvesribeiro galvesribeiro self-requested a review July 2, 2019 22:00
Copy link
Member

@galvesribeiro galvesribeiro left a comment

Choose a reason for hiding this comment

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

Just fix the CI scripts and we're good to go! Thanks!

@theprateik
Copy link
Contributor Author

@galvesribeiro, thanks. I will try that and submit soon.

@theprateik
Copy link
Contributor Author

@galvesribeiro, fixed the CI scripts.

@theprateik
Copy link
Contributor Author

however, I am not sure if it is a bad idea to use newtonsoft.json instead of system.text.json and I hope that that you can give your thoughts on this. What do you think?

@djinnet, it seems .NET Core development team is pushing on system.text.json. So, I think this is the way to go.
This could be something that can be discussed though. Maybe an abstraction could be created for json serializer, so that developer could pick a preferred serializer between these two. Again, this could be totally unnecessary since system.text.json can become self-sufficient in the future.

@galvesribeiro
Copy link
Member

I would stick with the new JSON stuff.

The official SignalR client and the Hub uses it by default now, so I wouldn't move the other way around specially that now we're talking about javascript interop in this particular case.

If we ever come to make this "native" in C# only, perhaps we could add the right abstraction...

@galvesribeiro galvesribeiro merged commit df584ca into BlazorExtensions:master Jul 3, 2019
@galvesribeiro
Copy link
Member

@theprateik thank YOU so much for the PR!

Will publish the package soon!

galvesribeiro added a commit that referenced this pull request Jul 3, 2019
* fix example for injected HubConnectionBuilder (#28)

* Update to Blazor 3.0.0-preview6 (#35)

* Update to Blazor 3.0.0-preview6

- Update global.json to reflect the updated sdk version
- Replace Json.Deserialize by JsonSerializer.Parse in class HubConnection
- Add this. where missing in class HubConnection
- Replace onclick and bind attributes in test razor pages by @OnClick and @Bind
- Replace @functions by @code in test razor files
- Remove UseSignalR and UseBlazor from test Server Startup class Configure method
- Add UseAuthorization to Startup class
- Comment out AddMessagePackProtocol and uncommented AddJsonProtocol in Startup class
- Comment out AddMessagePackProtocol in ChatComponent class

* Update .vsts-ci.yml and .vsts-release.yml to use the .NET Core Preview 6 SDK.
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.

None yet

3 participants