-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: remote-only deletes now supported #220
Conversation
src/commands/force/source/delete.ts
Outdated
@@ -174,7 +176,7 @@ export class Delete extends DeployCommand { | |||
|
|||
private deleteFilesLocally(): void { | |||
if (!this.getFlag('checkonly') && getString(this.deployResult, 'response.status') === 'Succeeded') { | |||
this.sourceComponents.map((component) => { | |||
this.componentSet.toArray().map((component: SourceComponent) => { |
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.
componentSet.toArray is going to have to iterate the whole thing (and it's a generator so it has to call next() on all of them) https://github.com/forcedotcom/source-deploy-retrieve/blob/aa79e2642a7aa35b05cb71c9f4cc0b0c21c21123/src/collections/lazyCollection.ts#L59
Perf idea: do that once and store it somewhere (ex: this.componentSetAsArray
) to use in the various places.
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.
approved. I'm not particularly worried about perf on delete
as much as the commands where I expected a lot more sourceMembers.
@@ -174,7 +176,7 @@ export class Delete extends DeployCommand { | |||
|
|||
private deleteFilesLocally(): void { | |||
if (!this.getFlag('checkonly') && getString(this.deployResult, 'response.status') === 'Succeeded') { | |||
this.sourceComponents.map((component) => { | |||
this.componentSet.toArray().map((component: SourceComponent) => { | |||
// delete the content and/or the xml of the components | |||
if (component.content) { | |||
const stats = fs.lstatSync(component.content); |
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'd also suggest thinking through if it's possible to parallelize these via Promise.all (ex: read all the files and build the file list of what needs to be deleted, then await all those deletes in a Promise.all).
I'm thinking of our windows fs pains. Again, perhaps not a big deal for deletes.
This is looking good @WillieRuemmele. QA'd this with the old plugin-source and then with your changes. Tried the following and all looks good: Retrieved w/ prompts |
What does this PR do?
source:delete
would fail when trying to delete metadata that was only present in the org, and not locallynow it will delete remote-only metadata
What issues does this PR fix or reference?
@W-9892375@