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

Feat: Full server-side pagination for important events #3515

Merged
merged 8 commits into from
Sep 23, 2023

Conversation

chakflying
Copy link
Collaborator

@chakflying chakflying commented Aug 2, 2023

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

  • Remove the use of sendImportantHeartbeatList
  • There is no longer a global state of importantHeartbeatList in vue
  • Implement server-side pagination, where on initial load, the view request the total number of items, then each page of items is only loaded as they are selected
  • Use a global event bus mitt to pass new important heartbeats to any list currently active

The code change is relatively simple, but the performance improvement should be significant.

Fixes #726
Fixes #758
Fixes #3497
Fixes #3494
Fixes #3127
Fixes #1716
Fixes #1397
Fixes #2346

Type of change

  • Performance optimization

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Overall, very clean. I have left some minor inline comments.

One question:
Why did you add mitt exactly? (instead of function calls)

server/server.js Outdated Show resolved Hide resolved
server/server.js Outdated Show resolved Hide resolved
src/pages/DashboardHome.vue Show resolved Hide resolved
@@ -354,9 +340,23 @@ export default {
return getResBaseURL() + this.monitor.screenshot + "?time=" + this.cacheTime;
}
},

watch: {
page(to) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the reactivity in this case correct? (am I missing something?) 🤔

Let's assume that monitor=null ⇒ the code is skipped.
If I then monitor gets populated ⇒ getImportantHeartbeatListPaged is never called or am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. This code normally works because AFAIK in the vue lifecycle computed runs before mounted, so this.monitor should be true. I added a watch for monitor in case the monitorList has not been loaded when we are mounted for some reason.

@chakflying
Copy link
Collaborator Author

we need to send an event from the root of the vue app to deep inside the component tree Details.vue. I'm not aware of any function calls that can do this.

I looked through the alternatives suggested in the Vue 3 Docs but none of them seemed helpful, so the global event bus option was chosen.

@chakflying chakflying force-pushed the feat/important-beats-serverside branch from 27fc2a5 to 285320d Compare August 16, 2023 08:01
@chakflying chakflying changed the base branch from master to 2.0.X August 16, 2023 08:01
@chakflying chakflying mentioned this pull request Aug 17, 2023
2 tasks
@CommanderStorm
Copy link
Collaborator

I found this issue, which is likely fixed by this PR:
#758

@louislam louislam changed the base branch from 2.0.X to master August 31, 2023 21:34
@louislam louislam merged commit 7c49f7e into louislam:master Sep 23, 2023
12 of 14 checks passed
@Saibamen
Copy link
Contributor

Could this be ported to 1.23.x branch?

@CommanderStorm
Copy link
Collaborator

Given how many performance improvements have gone into v2, I would argue that migrating to v2 would be a better choice for our userbase.

=> I think classifying speed as a feature instead of a bug is very reasonable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment