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

Implement SSE notifications #9451

Merged
merged 29 commits into from
Aug 3, 2023
Merged

Implement SSE notifications #9451

merged 29 commits into from
Aug 3, 2023

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Jul 20, 2023

Description

We implemented basic SSE to get notifications instantly from the server without requesting all the time.
See #9434

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented Jul 20, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@lookacat lookacat changed the title Implement PoC Polyfill Implement PoC SSE Polyfill Jul 20, 2023
@lookacat lookacat marked this pull request as ready for review July 21, 2023 13:53
@lookacat lookacat changed the title Implement PoC SSE Polyfill Implement SSE notifications Jul 21, 2023
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Could you please elaborate a bit how it works in detail? 🙂 I'm especially interested in the token handling, what happens during token renewal? I got a 401 error from the SEE endpoint after a short while, not sure though if that was related to token renewal.

Also it doesn't work with pnpm vite for me because it always tries to connect to the wrong port (9201 instead of 9200).

package.json Outdated Show resolved Hide resolved
@lookacat
Copy link
Contributor Author

@JammingBen i talked to julian, he told me the connection stays open even if the token is no longer valid. Its weird that you get an 401, i will try to replicate it

@lookacat lookacat requested a review from JammingBen July 27, 2023 09:33
@lookacat
Copy link
Contributor Author

@JammingBen i don't get the 401 anymore at least with vite, also port 9201 should work as well as 9200

@lookacat lookacat force-pushed the sse-notifications branch from 1916cb8 to 9930747 Compare July 27, 2023 12:40
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

I still have 2 concerns:

  • What happens if a user login is actually invalid and the server correctly returns 401? This solution will constantly try the re-establish the connection, right? Can we add some kind of retry-limit?
  • An established connection won't change when the user changes their language. I assume the messages from the server come based on the Accept-Language header? In that case we also need to re-establish the connection with the updated language.

@lookacat lookacat requested a review from JammingBen July 31, 2023 07:47
@lookacat lookacat force-pushed the sse-notifications branch from 03dab99 to 4ee4a41 Compare July 31, 2023 09:07
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

I still have 2 issues:

  • When changing the language, existing SSE connections don't get terminated. You can see this by changing the language multiple times and then triggering a notification -> there will be multiple notifications.
  • Notifications are not being fetched when the tab is inactive, even if I switch back to the tab. I guess we need to call the regular notifications endpoint each time we open a connection. Maybe you can do this in onOpen?

@lookacat
Copy link
Contributor Author

lookacat commented Aug 2, 2023

alright good findings

@lookacat lookacat requested a review from JammingBen August 2, 2023 13:23
@sonarcloud
Copy link

sonarcloud bot commented Aug 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

46.5% 46.5% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Nice, feels very good now! 👍

@JammingBen JammingBen merged commit 81aeaa2 into master Aug 3, 2023
@delete-merged-branch delete-merged-branch bot deleted the sse-notifications branch August 3, 2023 11:48
AlexAndBear pushed a commit that referenced this pull request Aug 15, 2023
* Implement PoC Polyfill
AlexAndBear pushed a commit that referenced this pull request Dec 13, 2023
* Implement PoC Polyfill
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.

[web] Show Notification Instantly
2 participants