pr feedback

Author: mrgrain

packages/@aws-cdk/mixins-preview/lib/core/applicator.ts

@@ -1,5 +1,5 @@
 import type { IConstruct } from 'constructs';
-import { UnscopedValidationError, ValidationError } from 'aws-cdk-lib/core';
+import { ValidationError } from 'aws-cdk-lib/core';
 import type { IMixin } from './mixins';
 import { ConstructSelector } from './selectors';
 
@@ -25,11 +25,11 @@ export class MixinApplicator {
     const constructs = this.selector.select(this.scope);
     for (const construct of constructs) {
       if (mixin.supports(construct)) {
-        mixin.applyTo(construct);
         const errors = mixin.validate?.(construct) ?? [];
         if (errors.length > 0) {
-          throw new ValidationError(`Mixin validation failed: ${errors.join(', ')}`, construct);
+          throw new ValidationError(`Mixin validation failed: ${errors.join(', ')}`, this.scope);
         }
+        mixin.applyTo(construct);
       }
     }
     return this;
@@ -43,16 +43,16 @@ export class MixinApplicator {
     let applied = false;
     for (const construct of constructs) {
       if (mixin.supports(construct)) {
-        mixin.applyTo(construct);
         const errors = mixin.validate?.(construct) ?? [];
         if (errors.length > 0) {
           throw new ValidationError(`Mixin validation failed: ${errors.join(', ')}`, construct);
         }
+        mixin.applyTo(construct);
         applied = true;
       }
     }
     if (!applied) {
-      throw new UnscopedValidationError(`Mixin ${mixin.constructor.name} could not be applied to any constructs`);
+      throw new ValidationError(`Mixin ${mixin.constructor.name} could not be applied to any constructs`, this.scope);
     }
     return this;
   }

packages/@aws-cdk/mixins-preview/lib/core/mixins.ts

@@ -2,6 +2,9 @@ import type { IConstruct } from 'constructs';
 import type { ConstructSelector } from './selectors';
 import { MixinApplicator } from './applicator';
 
+// this will change when we update the interface to deliberately break compatibility checks
+const MIXIN_SYMBOL = Symbol.for('@aws-cdk/mixins-preview.Mixin.pre1');
+
 /**
  * Main entry point for applying mixins.
  */
@@ -39,6 +42,20 @@ export interface IMixin {
  * Abstract base class for mixins that provides default implementations.
  */
 export abstract class Mixin implements IMixin {
+  /**
+   * Checks if `x` is a Mixin.
+   *
+   * @param x Any object
+   * @returns true if `x` is an object created from a class which extends `Mixin`.
+   */
+  static isMixin(x: any): x is Mixin {
+    return x != null && typeof x === 'object' && MIXIN_SYMBOL in x;
+  }
+
+  constructor() {
+    Object.defineProperty(this, MIXIN_SYMBOL, { value: true });
+  }
+
   public supports(_construct: IConstruct): boolean {
     return true;
   }

packages/@aws-cdk/mixins-preview/lib/core/selectors.ts

@@ -41,15 +41,7 @@ export abstract class ConstructSelector {
 
 class AllConstructsSelector extends ConstructSelector {
   select(scope: IConstruct): IConstruct[] {
-    const result: IConstruct[] = [];
-    const visit = (node: IConstruct) => {
-      result.push(node);
-      for (const child of node.node.children) {
-        visit(child);
-      }
-    };
-    visit(scope);
-    return result;
+    return scope.node.findAll();
   }
 }
 

packages/@aws-cdk/mixins-preview/lib/util/property-mixins.ts

@@ -21,7 +21,7 @@ export function deepMerge(target: any, source: any, mergeOnly: string[]): any {
 
     if (typeof sourceValue === 'object' && sourceValue != null && !Array.isArray(sourceValue) &&
         typeof targetValue === 'object' && targetValue != null && !Array.isArray(targetValue)) {
-      target[key] = deepMergeCopy({}, targetValue, sourceValue);
+      target[key] = deepMergeCopy(Object.create(null), targetValue, sourceValue);
     } else {
       target[key] = sourceValue;
     }
@@ -58,6 +58,9 @@ const MERGE_EXCLUDE_KEYS: string[] = [
 ];
 
 /**
+ * This is an unchanged copy from packages/aws-cdk-lib/core/lib/cfn-resource.ts
+ * The intention will be to use this function from core once the package is merged.
+ *
  * Merges `source` into `target`, overriding any existing values.
  * `null`s will cause a value to be deleted.
  */
@@ -68,7 +71,7 @@ function deepMergeCopy(target: any, ...sources: any[]) {
     }
 
     for (const key of Object.keys(source)) {
-      if (key === '__proto__' || key === 'constructor') {
+      if (key === '__proto__' || key === 'constructor' || key === 'prototype') {
         continue;
       }
 

packages/@aws-cdk/mixins-preview/test/mixin.test.ts

@@ -26,3 +26,18 @@ describe('IMixin', () => {
     expect((result as any).mixinApplied).toBe(true);
   });
 });
+
+describe('Mixin', () => {
+  test('isMixin returns true for Mixin instances', () => {
+    const mixin = new TestMixin();
+    expect(Mixin.isMixin(mixin)).toBe(true);
+  });
+
+  test('isMixin returns false for non-Mixin objects', () => {
+    expect(Mixin.isMixin({})).toBe(false);
+    expect(Mixin.isMixin(null)).toBe(false);
+    expect(Mixin.isMixin(undefined)).toBe(false);
+    expect(Mixin.isMixin('string')).toBe(false);
+    expect(Mixin.isMixin(123)).toBe(false);
+  });
+});

packages/aws-cdk-lib/aws-ec2/lib/cfn-init.ts

@@ -302,7 +302,7 @@ function deepMerge(target?: Record<string, any>, src?: Record<string, any>) {
   if (src == null) { return target; }
 
   for (const [key, value] of Object.entries(src)) {
-    if (key === '__proto__' || key === 'constructor') {
+    if (key === '__proto__' || key === 'constructor' || key === 'prototype') {
       continue;
     }
 

packages/aws-cdk-lib/core/lib/cfn-resource.ts

@@ -661,7 +661,7 @@ function deepMerge(target: any, ...sources: any[]) {
     }
 
     for (const key of Object.keys(source)) {
-      if (key === '__proto__' || key === 'constructor') {
+      if (key === '__proto__' || key === 'constructor' || key === 'prototype') {
         continue;
       }