BREAKING CHANGE Wr/destructive changes #450

WillieRuemmele · 2021-09-13T22:20:20Z

What does this PR do?

BREAKING CHANGE:
breaks a few public methods' parameters
adds pre/post destructive change support to ComponentSet.fromManifest

What issues does this PR fix or reference?

@W-9892375@

Functionality Before

destructive component sets were limited to a single type (pre/post)

Functionality After

you can now specify which type of deletion per SourceComponent

WillieRuemmele · 2021-09-13T23:17:28Z

src/resolve/sourceComponent.ts

+    return this.destructiveChangesType;
+  }
+
+  public setMarkedForDelete(destructiveChangeType?: DestructiveChangesType | boolean): void {


as a setter, we need to be able to specify if it's a deletion (true/false) and what type of deletionDestructiveChangesType passing nothing will set it to true and a post delete

WillieRuemmele · 2021-09-13T23:18:35Z

src/collections/componentSet.ts

@@ -360,7 +402,7 @@ export class ComponentSet extends LazyCollection<MetadataComponent> {
    >;
  }

-  public add(component: ComponentLike, asDeletion?: boolean): void {
+  public add(component: ComponentLike, deletionType?: DestructiveChangesType): void {


replacing the asDeletion param is the breaking change, which can also be seen on other methods

WillieRuemmele · 2021-09-13T23:19:05Z

src/collections/componentSet.ts

+      }
+    };
+
+    for (const component of manifest.components) {


write the manifest comps. and then the destructive post (if present) and the destructive pre (if present)

jag-j · 2021-09-20T16:39:02Z

test/convert/metadataConverter.test.ts

@@ -391,21 +404,21 @@ describe('MetadataConverter', () => {
      ]);
    });

-    it('should write destructive changes pre manifest when ComponentSet has deletes', async () => {
+    it('should write destructive changes pre manifest when ComponentSet has deletes marked for pre', async () => {


Nit pick. Can you rename the similar test for post in line 167?

jeffb-sfdc · 2021-09-20T20:09:50Z

src/collections/componentSet.ts

@@ -76,11 +79,11 @@ export class ComponentSet extends LazyCollection<MetadataComponent> {
    this.logger = Logger.childFromRoot(this.constructor.name);

    for (const component of components) {
-      let asDeletion = false;
+      let destructiveType = this.destructiveChangesType;


Since this is an "if-else" assignment, a turnery operation can be used here.

const destructiveType = (component instanceof SourceComponent) ? component.getDestructiveChangesType(), : this.destructiveChangesType;

jeffb-sfdc · 2021-09-20T20:17:29Z

src/collections/componentSet.ts

      for (const path of paths) {
        for (const component of resolver.getComponentsFromPath(path, inclusiveFilter)) {
-          set.add(component, asDeletes);
+          set.add(component, destructiveType);


Just dbl-checking here, as I'm not clear on the code paths - since destructiveType is nullable, what happens when set.add(component, null) is called and then later used?

it should default to a POST i.e. generating a destructiveChangesPost.xml

jeffb-sfdc · 2021-09-20T20:41:19Z

src/collections/componentSet.ts

    // If this ComponentSet has components marked for delete, we need to
    // only include those components in a destructiveChanges.xml and
    // all other components in the regular manifest.
    let components = this.components;
    if (this.hasDeletes) {
-      if (forDestructiveChanges) {
-        components = this.destructiveComponents;
+      if (destructiveType) {


A turnery operation can be used here

mshanemc · 2021-09-21T15:31:34Z

src/collections/componentSet.ts

@@ -168,23 +171,62 @@ export class ComponentSet extends LazyCollection<MetadataComponent> {
  public static async fromManifest(input: string | FromManifestOptions): Promise<ComponentSet> {
    const manifestPath = typeof input === 'string' ? input : input.manifestPath;
    const options = (typeof input === 'object' ? input : {}) as Partial<FromManifestOptions>;
+    let manifestPre;
+    let manifestPost;
+    const hasManifestPre = !!options.destructivePre;


Could the conditionals below could refer back to options.destructivePre/Post for truthiness, rather than creating extra constants?

mshanemc · 2021-09-21T15:35:16Z

src/collections/componentSet.ts


    const manifestResolver = new ManifestResolver(options.tree, options.registry);
    const manifest = await manifestResolver.resolve(manifestPath);
+    if (hasManifestPre) {
+      manifestPre = await manifestResolver.resolve(options.destructivePre);


suggest

const manifestPre = options.destructivePost ? await manifestResolver.resolve(options.destructivePre) : undefined;

then you could even refer to the truthiness of manifestPre further down

it's cleaner if everything can stay const

mshanemc

most of the comments are just style stuff and concision or ideas.

setMarkedForDelete and getTypesOfDestructiveChanges are the only actual problems I saw.

mshanemc · 2021-09-21T15:43:22Z

src/collections/componentSet.ts

-        if (!this.destructiveComponents.has(key)) {
-          this.destructiveComponents.set(key, new Map<string, SourceComponent>());
+        const deletions = this.destructiveComponents[deletionType];
+        if (!deletions.has(key)) {


I have some feelings about 3 levels of if

since we don't do anything for not if (component instanceof SourceComponent) we could flip that and return;

the non deletionType is simple...we could do that and then return (currently 424-427);

then handle all the delete logic (the least likely branch to get used)

mshanemc · 2021-09-21T16:02:11Z

src/collections/componentSet.ts

+      // something could try adding a component meant for deletion improperly, which would be marked as an addition
+      // specifically the ComponentSet.fromManifest with the `resolveSourcePaths` options which calls
+      // ComponentSet.fromSource, and adds everything as an addition
+      if (


oh, maybe I understand the flow logic now that I read down to here. Maybe this could be a function that could run on the delete and non-delete path?

Or those paths could check for existence on its counterpart (deletion would delete it from manifestComponents and non-deletion would just not add it if it's already in deletions?)

mshanemc · 2021-09-21T16:13:22Z

src/collections/componentSet.ts

+   *
+   * @return DestructiveChangesType[]
+   */
+  public getTypesOfDestructiveChanges(): DestructiveChangesType[] {


hasDeletes is doing pretty much the same logic, and is checked in both the Converter and CS's getObject.

This could also return an empty array for "there are none" so there is only one.

As written, if you were to NOT check hasDeletes before asking getTypesOfDestructiveChanges, your CS would tell that both types are present (the else condition here).

suggestion: make an array, and conditionally push to it (there might be 0,1, or 2 values).

then we can eliminate the hasDeletes since this code would have no effect for an empty array.

const destructiveChangesTypes = cs.getTypesOfDestructiveChanges(); destructiveChangesTypes.map((destructiveChangesType) => {

mshanemc · 2021-09-21T16:14:58Z

src/convert/metadataConverter.ts

@@ -103,13 +102,19 @@ export class MetadataConverter {
          if (!isSource) {
            const manifestPath = join(packagePath, MetadataConverter.PACKAGE_XML_FILE);
            tasks.push(promises.writeFile(manifestPath, manifestContents));
-
            // For deploying destructive changes
            if (cs.hasDeletes) {


see comments on CS about hasDeletes and getTypesOfDestructiveChanges

mshanemc · 2021-09-21T16:36:02Z

src/resolve/sourceComponent.ts

+    ) {
+      this.markedForDelete = true;
+      this.destructiveChangesType =
+        (destructiveChangeType as DestructiveChangesType) || DestructiveChangesType.POST;


if true is passed in, you're making an invalid type assertion true as DestructiveChangesType.

What all would the else condition handle? false only? If so, maybe invert the order and return early?

then you could have this.markedForDelete = true (because it's not false)
and then everything except PRE would mean POST, right?

mshanemc · 2021-09-21T16:38:21Z

test/resolve/sourceComponent.test.ts

@@ -60,7 +60,7 @@ describe('SourceComponent', () => {
  it('should return correct markedForDelete status', () => {
    expect(COMPONENT.isMarkedForDelete()).to.be.false;
    try {


this would be a good place to try out the other set values (destructive types, true)

mshanemc

most of the comments are just style stuff and concision or ideas.

setMarkedForDelete and getTypesOfDestructiveChanges are the only actual problems I saw.

shetzel · 2021-09-24T17:25:25Z

src/collections/componentSet.ts

+            name: component.fullName,
+          }),
+          DestructiveChangesType.POST
+        );


This method has doubled in size and I think we can make it more DRY by extracting some of the destructive changes logic into a private method. The above conditionals are nearly identical.

Suggested change

);

const resolveDestructiveChanges = (option, type) => {

const manifest = await manifestResolver.resolve(option);

for (const comp of manifest.components) {

addComponent(new SourceComponent({ type: comp.type, name: comp.fullName }), type);

}

};

if (options.destructivePre) {

resolveDestructiveChanges(options.destructivePre, DestructiveChangesType.PRE);

}

if (options.destructivePost) {

resolveDestructiveChanges(options.destructivePost, DestructiveChangesType.POST);

}

src/collections/componentSet.ts

shetzel · 2021-09-24T17:48:14Z

src/convert/metadataConverter.ts

-              const destructiveManifestPath = join(packagePath, manifestFileName);
-              tasks.push(promises.writeFile(destructiveManifestPath, destructiveManifestContents));
+            const destructiveChangesTypes = cs.getTypesOfDestructiveChanges();
+            if (destructiveChangesTypes) {


This could be an empty array, which is truthy so we should instead do if (destructiveChangesTypes.length)

src/convert/metadataConverter.ts

salesforce-cla · 2021-10-12T14:48:04Z

Thanks for the contribution! Unfortunately we can't verify the commit author(s): William Ruemmele <w***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

WillieRuemmele added 4 commits September 9, 2021 16:15

fix: inital POC for destructiveChangesPre and Post

4c62a7e

chore: pre/post destructive changes working, UT failing

81d9eb2

chore: pre/post destructive changes working, UT passing

9625604

refactor: remove asDelete param from methods, using deletion type

18c5b5f

WillieRuemmele requested review from a team as code owners September 13, 2021 22:20

WillieRuemmele requested a review from jeffb-sfdc September 13, 2021 22:20

Merge branch 'main' into wr/destructiveChanges

9eea66f

WillieRuemmele changed the title ~~Wr/destructive changes~~ BREAKING CHANGE Wr/destructive changes Sep 13, 2021

WillieRuemmele commented Sep 13, 2021

View reviewed changes

jag-j reviewed Sep 20, 2021

View reviewed changes

jeffb-sfdc reviewed Sep 20, 2021

View reviewed changes

mshanemc reviewed Sep 21, 2021

View reviewed changes

mshanemc requested changes Sep 21, 2021

View reviewed changes

WillieRuemmele added 2 commits September 21, 2021 20:57

chore: code review I - clean up ComponentSet

fc53ee3

Merge branch 'main' into wr/destructiveChanges

200508c

shetzel requested changes Sep 24, 2021

View reviewed changes

WillieRuemmele added 3 commits September 24, 2021 13:46

Merge branch 'main' into wr/destructiveChanges

3fa4cdc

chore: most of Steve's changes made

238c041

chore: steve's second change

9661bed

shetzel approved these changes Sep 24, 2021

View reviewed changes

Merge branch 'main' into wr/destructiveChanges

ef1e6a0

salesforce-cla bot added the cla:missing label Oct 12, 2021

chore: actually resolve conflicts

953b9b5

WillieRuemmele force-pushed the wr/destructiveChanges branch from 2cec4f4 to 953b9b5 Compare October 12, 2021 14:48

salesforce-cla bot removed the cla:missing label Oct 12, 2021

Merge branch 'main' into wr/destructiveChanges

ea00ffa

WillieRuemmele mentioned this pull request Oct 12, 2021

Wr/destructive deploy salesforcecli/plugin-source#230

Merged

mshanemc self-requested a review October 13, 2021 19:16

mshanemc approved these changes Oct 13, 2021

View reviewed changes

Merge branch 'main' into wr/destructiveChanges

29519ea

WillieRuemmele merged commit 0f67112 into main Oct 21, 2021

WillieRuemmele deleted the wr/destructiveChanges branch October 21, 2021 14:28

randi274 mentioned this pull request Oct 27, 2021

chore: bump SDR forcedotcom/salesforcedx-vscode#3622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING CHANGE Wr/destructive changes #450

BREAKING CHANGE Wr/destructive changes #450

WillieRuemmele commented Sep 13, 2021 •

edited

Loading

WillieRuemmele Sep 13, 2021

WillieRuemmele Sep 13, 2021

WillieRuemmele Sep 13, 2021

jag-j Sep 20, 2021

jeffb-sfdc Sep 20, 2021

jeffb-sfdc Sep 20, 2021

WillieRuemmele Sep 20, 2021

jeffb-sfdc Sep 20, 2021

mshanemc Sep 21, 2021

mshanemc Sep 21, 2021

mshanemc left a comment

mshanemc Sep 21, 2021

mshanemc Sep 21, 2021

mshanemc Sep 21, 2021

mshanemc Sep 21, 2021

mshanemc Sep 21, 2021

mshanemc Sep 21, 2021

mshanemc left a comment

shetzel Sep 24, 2021

shetzel Sep 24, 2021

shetzel Sep 24, 2021

salesforce-cla bot commented Oct 12, 2021

-        );
+const resolveDestructiveChanges = (option, type) => {
+  const manifest = await manifestResolver.resolve(option);
+  for (const comp of manifest.components) {
+      addComponent(new SourceComponent({ type: comp.type, name: comp.fullName }), type);
+  }
+};
+if (options.destructivePre) {
+  resolveDestructiveChanges(options.destructivePre, DestructiveChangesType.PRE);
+}
+if (options.destructivePost) {
+  resolveDestructiveChanges(options.destructivePost, DestructiveChangesType.POST);
+}

BREAKING CHANGE Wr/destructive changes #450

BREAKING CHANGE Wr/destructive changes #450

Conversation

WillieRuemmele commented Sep 13, 2021 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Functionality Before

Functionality After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salesforce-cla bot commented Oct 12, 2021

WillieRuemmele commented Sep 13, 2021 •

edited

Loading