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: project tree view #1390

Merged
merged 27 commits into from
Nov 1, 2022

Conversation

NicoVogel
Copy link
Contributor

@NicoVogel NicoVogel commented Oct 21, 2022

An implementation for the request #1181.

Changes:

  • extract the logic to create the project view into separate classes (one for list and a new one for tree)
  • change the code names (functions, variables) to be closer to what it actually is
  • separate the view items into different types (folder, project, target) to make the code more readable

I know that there is already a PR for it, but A) I could not simply extend the existing PR to what was required and B) there is no guarantee that even if I extend the other PR, that the person will actually merge my suggestions.

link to the other PR #1338

Non the less, I want to thank @rickvandermey to get this feature going. Because without his involvement, I would probably not have implement the feature myself.

closes #1338
resolves #1181

@nx-cloud
Copy link

nx-cloud bot commented Oct 21, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2ee8d50. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 6 targets

Sent with 💌 from NxCloud.

Copy link
Collaborator

@MaxKless MaxKless left a comment

Choose a reason for hiding this comment

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

Hey this looks really good! I haven't been able to look at everything yet but some small stuff could be fixed. Good job tho looks great

@NicoVogel
Copy link
Contributor Author

I was also thing of changing the config setting from a boolean to a string (enum). In case that another view would be added.

One that I could think of would be grouped by tag.

Copy link
Collaborator

@MaxKless MaxKless left a comment

Choose a reason for hiding this comment

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

Thanks for your work. I think it's a great solution and will help many. Just a few things to fix :)

I also pushed a small commit renaming some functions

@NicoVogel
Copy link
Contributor Author

Thanks for your work. I think it's a great solution and will help many. Just a few things to fix :)

Just read through the review comments and I agree with your suggestions.

I was also wondering it it makes sense to cache the result of cliTaskProvider.getProjects() and only update it in case of a reload 🤔

@NicoVogel
Copy link
Contributor Author

@MaxKless I think I will skip the optimizations in this PR, and only focus on the feature for now.

I would rather create a second PR to address them and have the feature ready.

Is that fine for you?

@MaxKless
Copy link
Collaborator

Yes you can skip the memoization and extra features for now :) just the bugfix stuff and naming clarity I would like to see

@NicoVogel
Copy link
Contributor Author

If I did not miss anything, then I should be again ready for review 🙂

Copy link
Collaborator

@MaxKless MaxKless left a comment

Choose a reason for hiding this comment

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

Alright we're getting there!
Sadly, I've found one major problem with the new tree view: compatibility with angular workspaces. As you can see from the screenshot, the view is just empty currently.
To work on it, you can copy the testworkspace-ng folder from the vscode-e2e app and test it out!
image

I know I've been saying it a lot but thanks again for your work and the super fast iteration times!!

@NicoVogel
Copy link
Contributor Author

I have a suspicion that the tree view does not work for angular projects which have sub-projects.
So, I will investigate here as well

@NicoVogel
Copy link
Contributor Author

[...]
I know I've been saying it a lot but thanks again for your work and the super fast iteration times!!

Thank you for the nice words :)
But of course, I did not have time to address your feedback right after you wrote that :D

@NicoVogel
Copy link
Contributor Author

I have a suspicion that the tree view does not work for angular projects which have sub-projects. So, I will investigate here as well

As expected.
It works with a new setup like: https://github.com/saaivs/ng-multi-projects
But for the older ones it does not work, like: https://github.com/JoelViney/angular-multiple-applications-example

@NicoVogel
Copy link
Contributor Author

NicoVogel commented Oct 29, 2022

@MaxKless Do you also have any requirements regarding the git history?
I would squash everything that we did after my initial PR into the feat: implement project tree view commit.
Because I don't think that having all the adjustments as separate commits would be of any value in the future.

So, the PR will only have these 4 commits:
image

@Cammisuli
Copy link
Member

@NicoVogel we squash all prs into master. So whatever is set as the title of this PR will be the commit message that appears on master.

@NicoVogel NicoVogel force-pushed the feature/issue-1181-nested-projects branch from dc4e155 to 6a8359d Compare November 1, 2022 09:38
@NicoVogel
Copy link
Contributor Author

I have a suspicion that the tree view does not work for angular projects which have sub-projects. So, I will investigate here as well

As expected. It works with a new setup like: https://github.com/saaivs/ng-multi-projects But for the older ones it does not work, like: https://github.com/JoelViney/angular-multiple-applications-example

Fixed with the latest commit.

@MaxKless @Cammisuli Can you provide a fresh review?
Can you think of other cases which I might have missed in my latest implementation?

@Cammisuli
Copy link
Member

Oh, I think we need to bump the node version for the ci to get the path/win32 requires working for the tests. Give me a couple, and I'll update master and merge it into this branch.

@Cammisuli
Copy link
Member

So far everything looks good, thank you! I'm just going to do smoke test on windows and if everything runs well there, I'll merge it in.

Thank you for the contribution and your patience with our feedback 🙂

Copy link
Member

@Cammisuli Cammisuli left a comment

Choose a reason for hiding this comment

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

Thanks again!

@Cammisuli Cammisuli merged commit ea9445d into nrwl:master Nov 1, 2022
@NicoVogel
Copy link
Contributor Author

@MaxKless I validated the performance of the TreeView.
I used this blog as a base as I never tested the performance of a vscode extension: https://www.nicoespeon.com/en/2019/11/fix-vscode-extension-performance-issue/

Actions I did

  1. open nx-console in vscode
  2. start yarn watch
  3. start the extension with F5
  4. open nx-examples (make sure that tree view is used)
  5. start profiling
  6. open NX Project view
  7. navigate into the projects view
  8. click on reload button
  9. stop profiling

Measures

createRoot

image

  • not measurable how long it takes
  • only getProjects decides how long it takes

createFoldersOrProjectFromFolder

image

first measure

  • not measurable how long it takes
  • only getProjects decides how long it takes

second measure

  • takes 0.1ms (self)
  • getProjects takes about half of the total time

createTargetsFromProject

image

  • not measurable how long it takes
  • only getProjects decides how long it takes

same result 3x

Reload with tree already open

  • createRoot -> same as without reload
  • createFoldersOrProjectFromFolder -> same as first measure
  • createTargetsFromProject -> takes 0.1ms (self), getProjects takes about 2/3 of the total time
  • total time (checking the top most function) 3ms
    • there is some idle time before vscode calls again the getChildren function
    • It is called 3 times (for each level) and it takes 1ms each time

What now?

The tree view is more than fast enough!
Even improving the getProjects does not make sense, because the total aggregated time of all getProjects calls was 0.80ms.

@NicoVogel NicoVogel deleted the feature/issue-1181-nested-projects branch November 6, 2022 12:16
@Cammisuli
Copy link
Member

The whole of Nx console is about to be quicker with #1402 as well. Basically the longest running operation that plagued console is now handled in the language server, which is in a separate process 🙂

@MaxKless
Copy link
Collaborator

MaxKless commented Nov 7, 2022

Awesome @NicoVogel thx for all the effort in this PR. Really high quality contribution. I would give a shoutout on twitter but you don't have one do you? :O

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.

Feature Request - Grouping projects from Nx Console Projects section
3 participants