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.

Sign up for GitHub

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

Jump to bottom

Migrate SignalR feature into WebHost #121

Merged
merged 15 commits into from
May 14, 2020
Merged

Migrate SignalR feature into WebHost #121

merged 15 commits into from
May 14, 2020

Conversation

sondreb
Copy link
Member

@sondreb sondreb commented May 12, 2020

  • Migrate the SignalR feature into WebHost.
  • Further refactor WebHost from "Api" to "WebHost.
  • Remove the versioning support from REST API.
  • Enable flags to enable/disable UI, API and WS.
  • Generate and include XML documentation in the REST API for all features.
  • Add a basic example on Web Socket listener.

@sondreb
Migrate SignalR feature into WebHost
0ba0753 
- Migrate the SignalR feature into WebHost.
- Further refactor WebHost from "Api" to "WebHost.
- Remove the versioning support from REST API.
- Enable flags to enable/disable UI, API and WS.
- Generate and include XML documentation in the REST API for all features.
- Add a basic example on Web Socket listener.
@sondreb sondreb requested a review from dangershony May 12, 2020 01:01
dangershony
dangershony reviewed May 12, 2020
View reviewed changes
src/Features/Blockcore.Features.WebHost/WebHostSettings.cs Outdated
@@ -28,6 +28,21 @@ public class ApiSettings
/// <summary>URI to node's API interface.</summary>
public Timer KeepaliveTimer { get; private set; }

/// <summary>
/// If true then the node will add and start the SignalR feature. This should never be enabled if node is accessible to the public.
Copy link
Member

Choose a reason for hiding this comment

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

Are they on by default? should we document that?

Copy link
Member Author

Choose a reason for hiding this comment

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

this.EnableWS = config.GetOrDefault("enableWS", false, this.logger);
this.EnableUI = config.GetOrDefault("enableUI", true, this.logger);
this.EnableAPI = config.GetOrDefault("enableAPI", true, this.logger);

Probably document somewhere, and we should decide what we want on by default.

dangershony
dangershony reviewed May 12, 2020
View reviewed changes
Copy link
Member

@dangershony dangershony left a comment

Choose a reason for hiding this comment

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

Brilliant PR just the one comment to move broadcasters to their parent projects and this is good

sondreb and others added 5 commits May 13, 2020 20:41
@sondreb
Improve the Web Socket event handling
fb8315b 
- Return the EventBase directly to Web Socket consumers.
- Remove unused code.
- Remove references in WebHost project.
@MithrilMan
created a class to handle swagger api documentation xml file inclusion.
5e84889
@sondreb
Fix issue with exceptions during type loading
c15f86c 
- Move the Assembly extension from Swagger scaffolding to Utilities.
- Minor bug discovered in dispose when object is null (happened during an integration test).
@sondreb
Rename WebHost to NodeHost
ebd6cb9 
- Next commit will rename the physical folder.
@sondreb
Rename WebHost folder to NodeHost
2111993 
- Updates all references, etc. from WebHost to NodeHost.
dangershony
dangershony reviewed May 13, 2020
View reviewed changes
src/Blockcore/Base/BaseFeature.cs
@@ -311,8 +311,11 @@ private void StartAddressManager(NetworkPeerConnectionParameters connectionParam
/// <inheritdoc />
public override void Dispose()
{
this.logger.LogInformation("Flushing peers.");
this.flushAddressManagerLoop.Dispose();
if (this.flushAddressManagerLoop != null)
Copy link
Member

Choose a reason for hiding this comment

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

Fingers crossed

dangershony
dangershony reviewed May 14, 2020
View reviewed changes
Copy link
Member

@dangershony dangershony left a comment

Choose a reason for hiding this comment

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

Brilliant commit, I am gonna push a small change to fix the static service provider field.

dangershony and others added 6 commits May 14, 2020 01:32
@dangershony
Removing the dependency on a static service provider
e0eefad
@sondreb
Remove the Hash and Height
4f8af1b 
- We're returning the whole object in the evernt, no longer needed to read out hash and height.
@sondreb
Revert "Remove the Hash and Height"
b1794b4 
This reverts commit 4f8af1b.
@sondreb
Merge branch 'feature/migrate-signalr-to-webhost' of https://github.c…
e9643a0 
…om/block-core/blockcore into feature/migrate-signalr-to-webhost
@sondreb
Move the resolving of HubContext so it only runs when WS is enabled
f1bff6f 
- Minor cleanup of events, removing older properties.
@sondreb
Remove hardcoded URL to JS and WS endpoints
9aa82ca 
- Update to use relative path when hosted on the NodeHost.
- Add JsonIgnore on objects that can't serialize. This needs to be added to more.
sondreb added 2 commits May 14, 2020 13:30
@sondreb
Handle serialization of IPAddress, add ignore on structures that cras…
ac76f1b 
…h node

- Certain types will crash the node when sent to the Web Socket consumer.
- Beware of this issue in the future when working on the event types.
- Remove the EventType and instead only have EventName.
@sondreb
Fix dependency issue for broadcasters when no IEventsSubscriptionServ…
12b1983 
…ice is registered

- When using the node without .UseWebHost, no implementation of IEventsSubscriptionService is registered. This fixes the broadcasters by handling them as empty if one is not provided.
@sondreb
Update InitialBlockDownloadState.cs
1306285 
- Reverted the changes to check for nulls on IBD check.
@dangershony dangershony merged commit 7917a2f into master May 14, 2020
@dangershony dangershony deleted the feature/migrate-signalr-to-webhost branch May 14, 2020 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MithrilMan MithrilMan MithrilMan left review comments

@dangershony dangershony dangershony left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sondreb @MithrilMan @dangershony