-
Notifications
You must be signed in to change notification settings - Fork 214
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: Nx Move and Remove Project in Context Menu #1256
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e89a9e4. 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 targetsSent with 💌 from NxCloud. |
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.
I just took a quick look, and I see what you mean about the drilling down 😅 that WebView function is doing too much.
Either way, there's a few comments I have as well 😄
apps/vscode/src/package.json
Outdated
"group": "2_workspace" | ||
}, | ||
{ | ||
"when": "!isNxWorkspace && config.nxConsole.enableGenerateFromContextMenu", |
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.
I think it's better to use isAngularWorkspace
here.
"when": "!isNxWorkspace && config.nxConsole.enableGenerateFromContextMenu", | |
"when": "isAngularWorkspace && config.nxConsole.enableGenerateFromContextMenu", |
@@ -67,6 +75,20 @@ export function registerCliTaskCommands( | |||
selectCliCommandAndPromptForFlags('run', await getCliProjectFromUri(uri)) | |||
); | |||
|
|||
commands.registerCommand(`${cli}.move.fileexplorer`, async (uri: Uri) => { | |||
const getCorrectMoveGenerator = (uri: Uri) => '@nrwl/workspace:move'; |
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.
We could probably find out the minimum version of Nx that introduced this generator and have that logic here. We have this function here that could be exported and used outside that project:
export function nxVersion(): number | null { |
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.
Oooh, hadn't considered that. Good point.
For versions younger than we had the move/remove generators, we should probably remove this option from the context menu, right?
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.
Similar note: for when the user right-clicks on a node in the root, my solution would open the webview for the @nrwl/workspace:move
generator; without a project set.
I think this is problematic, since we should be using the @nrwl/angular:move
generator if the project is ends up being angular :\
@Cammisuli updates:
Taking off the WIP, lmk if anything looks like it's missing! |
In regards to the @nrwl/angular:move vs @nrwl/workspace:move point, there's also the case of community plugin projects which may require their move generator. e.g., I'm about to add a move generator to nx-dotnet to handle updating .csproj file paths in addition to moving the files ( see nx-dotnet/nx-dotnet#406 ). Do you think we could check for all installed plugins that have a generator called move, and prompt the user to choose between them? |
Hey @Cammisuli - wanted to get your thoughts on this approach before going too far in.
You can see the code for yourself, but the idea here is:
workspace:move
])I'm not a fan of the amount of "drilling" this approach takes, but I think it is the minimally invasive approach given our current state. (eventually it'd be nice to rework some of our APIs like we've discussed)