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

perf: adjust code fetching all projects #989

Merged
merged 2 commits into from
Jul 22, 2020
Merged

Conversation

lorenzo-cavazzi
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi commented Jul 22, 2020

This improves the client.getAllProjects function and limits its use to whenever it's necessary.

  • Prevent fetching user projects when browsing as an anonymous user.
  • Re-use client.getProjects function in client.getAllProjects and make it more readable.
  • Invoke client.getAllProjects also for starred projects to be sure we are not missing projects.

fix #986

EDIT: sorry for not making a preview available, I'm running out of test deployments 😞

… unnecessary

* take advantage of getProjects in getAllProjects.
* invoke getAllProjects for starred projects
* prevent fetching user projects when browsing as anonymous

fix #986
@lorenzo-cavazzi lorenzo-cavazzi requested a review from a team as a code owner July 22, 2020 12:37
@lorenzo-cavazzi lorenzo-cavazzi added this to the sprint-2020-07-09 milestone Jul 22, 2020
@lorenzo-cavazzi
Copy link
Member Author

While working on this, I came up 2 possible follow up -- I'll create the issues if they are relevant.

1.

We could further improve the projects fetching logic by:

  • separating the fetching variable for starred and member projects.
  • taking advantage of paging by fetching a few projects (5-10, enough to fill the preview in the Landing page), then lazy loading all the remaining. This would require a couple of extra control variables and minor adjustments to the current code.

The speedup is considerable only with a lot of member/starred projects (100+). We could probably address with lower priority.

2.

An interesting idea would be adding a global notification system so that we can popup info/ warning messages when something not too critical goes wrong.
E.G: errors in fetching starred projects currently result in .catch(error => []) without further actions. It's not easy to add visual feedback since it's invoked in many different places, and throwing an error is not desirable since the impact is limited. I would like to notify the user with a popup, possibly with a re-execute button. Ideally, I would add the warning to a global queue with a function like this: this.model.addNotification("cannot get the starred projects", "Type.WARNING", functionToReInvoke).
If this makes sense, I'll create an issue. We probably need anyway an extensive notification system when we have a web socket backend, this could be the first step.

@@ -96,9 +96,9 @@ function getFilesTreeLazy(client, files, projectId, openFilePath, lfsFiles) {

}

function addProjectMethods(client) {
async function addProjectMethods(client) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the addProjectMethods function should be synchronous.

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

👍

@lorenzo-cavazzi lorenzo-cavazzi merged commit 7a47e6c into master Jul 22, 2020
@lorenzo-cavazzi lorenzo-cavazzi deleted the 986-allprojects branch July 22, 2020 15:51
@ciyer
Copy link
Contributor

ciyer commented Jul 23, 2020

While working on this, I came up 2 possible follow up -- I'll create the issues if they are relevant.

I created #991 for the second suggestion. I think the first is also good, could you make an issue for that?

@lorenzo-cavazzi
Copy link
Member Author

lorenzo-cavazzi commented Jul 23, 2020

Great!
I created the other issue #993

ciyer pushed a commit that referenced this pull request Sep 9, 2020
* Take advantage of getProjects in getAllProjects.
* Invoke getAllProjects for starred projects.
* Prevent fetching user projects when browsing as anonymous.

fix #986

(cherry picked from commit 7a47e6c)
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.

Refactor getAllProjects
3 participants