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: Nested projects #1338

Conversation

rickvandermey
Copy link

  • [1181] create folder structure based on where to projects are located in the repository
  • feat: issue 1181 resolve some any typings

linked to #1181

at the moment its only possible when the folder has the same name as the lib/app. maybe should go deeper into that to fix that as well

Rick van der Meij added 3 commits September 8, 2022 14:27
@nx-cloud
Copy link

nx-cloud bot commented Sep 8, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 18bb367. 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 3 targets

Sent with 💌 from NxCloud.

@rickvandermey rickvandermey changed the title nested projects feat: Nested projects Sep 8, 2022
@Cammisuli Cammisuli self-assigned this Sep 8, 2022
@Cammisuli
Copy link
Member

Thanks for the PR @rickvandermey!

I've been testing it, and it appears that there's some weird artifacts that show up in the Nx Examples repo
image

Notice the 0,1, 2. In the previous view, it just didnt show anything/expand. Lets try and keep those behaviours the same

@rickvandermey
Copy link
Author

rickvandermey commented Sep 8, 2022

Thanks for the PR @rickvandermey!

I've been testing it, and it appears that there's some weird artifacts that show up in the Nx Examples repo image

Notice the 0,1, 2. In the previous view, it just didnt show anything/expand. Lets try and keep those behaviours the same

That was the note i added, the workspace project key isnt the same as the folder name
"shared-product-types": "libs/shared/product/types", -> "shared-product-types": "libs/shared/product/shared-product-types", that should fix it, thats the last part i have to add.

but because upcoming week i do not have a lot of spare time, I wanted to give you something to review, and maybe co-develop this feature

@Cammisuli
Copy link
Member

Ok, I'll reveiw it again once you add that fix in. We shouldn't have to make users change their config to show things in Nx Console. I'm fine with just hiding the project/child nodes if it doesnt match the path.

@rickvandermey
Copy link
Author

@Cammisuli I've updated the PR with working folder structure with tasks


export class NxProjectTreeHelper {

static async getProjectsInFolders(projects: [string, ProjectConfiguration][]): Promise<Record<string,any>> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a proper return type rather than any?

Comment on lines 12 to 17
splittedProject.reduce((r: {[key: string]: string;} & any, e: string, i: number) => {
if (splittedProject.length -1 > i) {
return r[e] || (r[e] = {});
}
return r[e] || (r[e] = project)
}, projectsObject)
Copy link
Member

Choose a reason for hiding this comment

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

This was a bit too confusing because of the variable names, and the modifying nested properties of the projectObject property. I feel like this (or something close to it) would make this a little more readable:

Suggested change
splittedProject.reduce((r: {[key: string]: string;} & any, e: string, i: number) => {
if (splittedProject.length -1 > i) {
return r[e] || (r[e] = {});
}
return r[e] || (r[e] = project)
}, projectsObject)
for (const [index, pathNode] of splittedProject.entries()) {
if (splittedProject.length - 1 > index) {
projectsObject[pathNode] ??= {};
continue;
}
projectsObject[pathNode] ??= project;
}

Copy link
Author

Choose a reason for hiding this comment

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

This was a bit too confusing because of the variable names, and the modifying nested properties of the projectObject property. I feel like this (or something close to it) would make this a little more readable:

I've tried implementing your code snippet, but it doesn't compile, so i've added functional naming for the parameters.

Copy link
Member

Choose a reason for hiding this comment

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

the variable names were one part of the confusion, but it's still modifying a nested object. There's a bunch of redirection happening with the nested object

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.

See my comments for individual sections. Also, would it be possible to clean up the icons here:
image

Basically remove the icon if it's a folder and try to open the project if there's no targets (like in assets). Right now both icons open the base package.json file, which doesnt make sense.

@rickvandermey
Copy link
Author

See my comments for individual sections. Also, would it be possible to clean up the icons here: image

Basically remove the icon if it's a folder and try to open the project if there's no targets (like in assets). Right now both icons open the base package.json file, which doesnt make sense.

I've added the same logic for projects so the same icons apply when a project. when its a folder, it still opens the package.json, I've tried to resolve to workspace.json instead, but have not figured it out (yet). Should be added in next snippet

if (nxProject.root === '') {
  item.contextValue = 'folder';
  return item;
}

return projectsObject;
}

static async findNestedObject(fields: any, key: string): Promise<any> {
Copy link
Member

Choose a reason for hiding this comment

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

I was hoping that with the change of the type in the previous function, that you would remove the any types from here as well 😅

Please make sure all new code are properly typed please 🙂

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.

2 participants