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

Server-Sent Events support - closes #2830 #2863

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

benfrancis
Copy link
Member

No description provided.

@benfrancis
Copy link
Member Author

Work-in-progress Server-Sent Events implementation for W3C WoT compliance (#2830), which is currently blocking #2806.

This still needs tests. It's a bit tricky to test manually because Postman doesn't support Server-Sent events, but you can use curl to see the raw output of the event stream.

Note that JavaScript Consumers using EventSource you will need to provide a JWT in a query string (as is currently the case with WebSockets), because the EventSource API doesn't allow sending an Authorization header. I've used the same approach in the example curl command below, you just need to insert your own JWT.

curl http://localhost:8080/things/virtual-things-10/events/virtualEvent?jwt={jwt} -H 'Accept: text/event-stream'

For testing you can use the Virtual Actions & Events Thing from the Virtual Things add-on, which emits an event when you invoke the "no input" action.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2021

Codecov Report

Merging #2863 (5bacefc) into master (f68be20) will increase coverage by 0.13%.
The diff coverage is 86.84%.

❗ Current head 5bacefc differs from pull request most recent head a52a63b. Consider uploading reports for the commit a52a63b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
+ Coverage   65.25%   65.39%   +0.13%     
==========================================
  Files         124      124              
  Lines        7952     7987      +35     
  Branches     1328     1332       +4     
==========================================
+ Hits         5189     5223      +34     
- Misses       2720     2721       +1     
  Partials       43       43              
Impacted Files Coverage Δ
src/router.ts 98.94% <ø> (ø)
src/controllers/events_controller.ts 89.58% <86.48%> (-10.42%) ⬇️
src/models/thing.ts 69.20% <100.00%> (+0.11%) ⬆️
src/controllers/internal_logs_controller.ts 84.21% <0.00%> (-1.76%) ⬇️
src/app.ts 56.32% <0.00%> (+2.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f68be20...a52a63b. Read the comment docs.

@benfrancis benfrancis changed the title WIP Server-Sent Events support - closes #2830 Server-Sent Events support - closes #2830 Aug 17, 2021
@benfrancis
Copy link
Member Author

OK, I've added integration tests. This is now ready for review.

@benfrancis benfrancis marked this pull request as ready for review August 18, 2021 12:04
@benfrancis
Copy link
Member Author

@relu91 I'd be interested in your review here too if you get chance.

Copy link
Collaborator

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

The PR looks good, aside from some comments below I'd refactor the unit test provided in smaller tests. I understand that logically they all belong to the same category (sse tests) but if we want to group them logically we can just use the describe function:

describe("Server Sent Event testing", () => {
  it("should subscribe", ()=>{})
  it("should subscribe to multiple events", ()=>{})
  it("should fail for not existing events", ()=>{})
}); 

What do you think?

src/controllers/events_controller.ts Outdated Show resolved Hide resolved
src/controllers/events_controller.ts Outdated Show resolved Hide resolved
src/test/integration/things-test.ts Outdated Show resolved Hide resolved
src/test/integration/things-test.ts Outdated Show resolved Hide resolved
src/test/integration/things-test.ts Outdated Show resolved Hide resolved
src/test/integration/things-test.ts Outdated Show resolved Hide resolved
src/test/integration/things-test.ts Outdated Show resolved Hide resolved
@benfrancis
Copy link
Member Author

Thanks for the review @relu91, great feedback. I think I've addressed all your comments.

Copy link
Collaborator

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Perfect 👍🏻

@benfrancis
Copy link
Member Author

@tim-hellhake agreed to make @relu91 a peer of the gateway module, so landing based on @relu91's review.

@tim-hellhake comments still welcome if you want to review.

@benfrancis benfrancis merged commit 2ce4a5a into WebThingsIO:master Aug 24, 2021
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