Skip to content

[#763] v_summary.js: ensure getFiltered() is only called once when reloading page#800

Merged
eugenepeh merged 14 commits intoreposense:masterfrom
jamessspanggg:763-redundant-get-filtered
Jul 20, 2019
Merged

[#763] v_summary.js: ensure getFiltered() is only called once when reloading page#800
eugenepeh merged 14 commits intoreposense:masterfrom
jamessspanggg:763-redundant-get-filtered

Conversation

@jamessspanggg
Copy link
Copy Markdown
Contributor

Fixes #763

When reloading the report page, the data in v_summary such as
filterSinceDate, filterUntilDate, filterTimeFrame etc will change based 
on the URL hash values. The changes in data will trigger the watchers 
for those data, and each of them will call getFiltered() method.

Only one call for getFiltered() is required after rendering the filter 
hash, the other getFiltered() calls by the watchers are redundant.
When the report is large in size, repeatedly executing getFiltered()
when reloading the report will also significantly affect the loading 
performance of our report.

Let's only allow watchers to execute getFiltered() after the first 
cycle of watcher updates to prevent redundant calls to getFiltered().

@jamessspanggg jamessspanggg changed the title [#763] v_summary.js: ensure getFiltered() is called only once when reloading page [#763] v_summary.js: ensure getFiltered() is only called once when reloading page Jul 10, 2019
@jamessspanggg jamessspanggg marked this pull request as ready for review July 10, 2019 04:05
@jamessspanggg jamessspanggg requested a review from a team July 10, 2019 04:06
Copy link
Copy Markdown
Member

@fzdy1914 fzdy1914 left a comment

Choose a reason for hiding this comment

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

I am wondering how can you judge how many times getFiltered is called?

@jamessspanggg
Copy link
Copy Markdown
Contributor Author

@fzdy1914 The number of modified toolbar controls. (Modified as in not the same as the default value)

By default, the watcher for tmpFilterSinceDate and tmpFilterUntilDate will be called as their default values are empty strings ‘ ‘.

So if the user changes group by to ‘groupByAuthors’ and granularity to ‘week’ (both are different compared to their default values), getFiltered will be called 5 times. (1 time for created() hook in v_summary, 2 times for tmpFilterSinceDate and tmpFilterUntilDate, and another 2 times for group by and granularity)

Copy link
Copy Markdown
Member

@fzdy1914 fzdy1914 left a comment

Choose a reason for hiding this comment

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

LGTM, I have tested, it indeed reduce the filter times from 3 to 1.

@fzdy1914 fzdy1914 requested a review from a team July 14, 2019 10:31
Comment thread frontend/src/static/js/v_summary.js Outdated
this.getFiltered();
if (this.isWatcherUpdated) {
this.getFiltered();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be method with a descriptive name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes I think it would be better to make this as a method.

@yong24s yong24s requested a review from eugenepeh July 17, 2019 06:07
Comment thread frontend/src/static/js/v_summary.js Outdated
},
getFilteredAfterWatcherUpdated() {
if (this.isWatcherUpdated) {
this.getFiltered();
Copy link
Copy Markdown
Member

@eugenepeh eugenepeh Jul 17, 2019

Choose a reason for hiding this comment

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

instead of doing this, why not just return in getFiltered if this.isWatcherUpdated is false instead?
It doesn't really quite make sense to name the method getFilteredAfterWatcherUpdated if all has to check for isWatcherUpdated

@eugenepeh eugenepeh requested a review from ongspxm July 18, 2019 11:42
@eugenepeh eugenepeh requested review from yong24s and removed request for yong24s July 18, 2019 11:42
@eugenepeh eugenepeh merged commit f652054 into reposense:master Jul 20, 2019
jamessspanggg added a commit to jamessspanggg/RepoSense that referenced this pull request Jul 22, 2019
eugenepeh pushed a commit that referenced this pull request Jul 24, 2019
…800)" (#832)

PR #800 was intended to ensure the getFiltered() method to be executed 
only once when the page is reloaded. The solution used was to set 
boolean canGetFiltered to true in beforeUpdate() lifecycle hook, 
preventing the execution of getFiltered() before the lifecycle hook.
However, the beforeUpdate() lifecycle hook will only be executed when 
there are changes to the DOM. 

Instances where the page is reloaded and there are no changes to the DOM
will not set boolean canGetFiltered to true, preventing getFiltered() to 
be executed until the DOM has changed.

Let's revert PR #800 to allow getFiltered() to be executed even when 
there are no changes to the DOM.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redundant getFiltered() method calls when reloading page

4 participants