Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
improve page load performance of large amount urls #5025
improve page load performance of large amount urls #5025
Changes from 3 commits
84f261c
32f2fe1
ae7f01b
bb499e2
96a1010
a81317f
b0281e4
043f1a2
cb1b414
59aea39
de48fe7
39f3e63
416ba88
4b3cfb6
5334823
f1577a4
ede8282
90dd60d
d321b77
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
.reduce
calls with object-changes here are really hard to parse for me and thus likely other devs.If there were a bug in here, I would not be able to tell.
Please move this into regular for-loops.
While moving this, please also check if
Map
is not more appropriate.I found this blog post with some claims that this might increase performance (if that code happends to be a factor we are spending a part of the remaining
700ms
..)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CommanderStorm
understand let me double check with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CommanderStorm
I simplified logic. can you review one more time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pretty strange
the whole part why you added
op
here and above seems to be to create a fast-path forOPERATIONS.DELETE
, right?Instead of doing this, please just remove the calls for
OPERATIONS.DELETE
if we really don't use thelist
there..Cleans up the code a lot..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CommanderStorm
its used as common method. based on this parameter we will identified operation & update same to frontend list. you can refer socket.js changes
while page load it will use OPERATIONS.LIST as default.
while insert or update. we will add only 1 URL object to list.
while delete no need to send list because we have to delete from fronted list. from backend already deleted.