From 8f9d2740d216bb04c35e0f8394e006b0e680a301 Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Tue, 29 Mar 2022 11:11:52 +0200 Subject: [PATCH] chore(cx-api): remove more `instanceof`s (#19511) Fixes #17974 Follow-up of #14468 This follows the implementation of https://github.com/aws/constructs/pull/955 to be more robust. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-ecr-assets/test/image-asset.test.ts | 6 +--- .../@aws-cdk/aws-s3-assets/test/asset.test.ts | 6 +--- .../lib/artifacts/asset-manifest-artifact.ts | 35 +++++++++++++++++++ .../lib/api/cloudformation-deployments.ts | 6 +--- 4 files changed, 38 insertions(+), 15 deletions(-) diff --git a/packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts b/packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts index d931401484ba2..7640557032510 100644 --- a/packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts +++ b/packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts @@ -452,7 +452,7 @@ testFutureBehavior('nested assemblies share assets: default synth edition', flag // Read the asset manifests to verify the file paths for (const stageName of ['Stage1', 'Stage2']) { - const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(isAssetManifestArtifact)[0]; + const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact)[0]; const manifest = JSON.parse(fs.readFileSync(manifestArtifact.file, { encoding: 'utf-8' })); expect(manifest.dockerImages[DEMO_IMAGE_ASSET_HASH].source).toEqual({ @@ -464,7 +464,3 @@ testFutureBehavior('nested assemblies share assets: default synth edition', flag function isStackArtifact(x: any): x is cxapi.CloudFormationStackArtifact { return x instanceof cxapi.CloudFormationStackArtifact; } - -function isAssetManifestArtifact(x: any): x is cxapi.AssetManifestArtifact { - return x instanceof cxapi.AssetManifestArtifact; -} diff --git a/packages/@aws-cdk/aws-s3-assets/test/asset.test.ts b/packages/@aws-cdk/aws-s3-assets/test/asset.test.ts index f04218f95929d..23666d832f7e2 100644 --- a/packages/@aws-cdk/aws-s3-assets/test/asset.test.ts +++ b/packages/@aws-cdk/aws-s3-assets/test/asset.test.ts @@ -275,7 +275,7 @@ test('nested assemblies share assets: default synth edition', () => { // Read the asset manifests to verify the file paths for (const stageName of ['Stage1', 'Stage2']) { - const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(isAssetManifestArtifact)[0]; + const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact)[0]; const manifest = JSON.parse(fs.readFileSync(manifestArtifact.file, { encoding: 'utf-8' })); expect(manifest.files[SAMPLE_ASSET_HASH].source).toEqual({ @@ -410,7 +410,3 @@ function mkdtempSync() { function isStackArtifact(x: any): x is cxapi.CloudFormationStackArtifact { return x instanceof cxapi.CloudFormationStackArtifact; } - -function isAssetManifestArtifact(x: any): x is cxapi.AssetManifestArtifact { - return x instanceof cxapi.AssetManifestArtifact; -} \ No newline at end of file diff --git a/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts b/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts index 3c8c102f2c5ab..10ddea69b624e 100644 --- a/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts +++ b/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts @@ -3,10 +3,33 @@ import * as cxschema from '@aws-cdk/cloud-assembly-schema'; import { CloudArtifact } from '../cloud-artifact'; import { CloudAssembly } from '../cloud-assembly'; +const ASSET_MANIFEST_ARTIFACT_SYM = Symbol.for('@aws-cdk/cx-api.AssetManifestArtifact'); + /** * Asset manifest is a description of a set of assets which need to be built and published */ export class AssetManifestArtifact extends CloudArtifact { + /** + * Checks if `art` is an instance of this class. + * + * Use this method instead of `instanceof` to properly detect `AssetManifestArtifact` + * instances, even when the construct library is symlinked. + * + * Explanation: in JavaScript, multiple copies of the `cx-api` library on + * disk are seen as independent, completely different libraries. As a + * consequence, the class `AssetManifestArtifact` in each copy of the `cx-api` library + * is seen as a different class, and an instance of one class will not test as + * `instanceof` the other class. `npm install` will not create installations + * like this, but users may manually symlink construct libraries together or + * use a monorepo tool: in those cases, multiple copies of the `cx-api` + * library can be accidentally installed, and `instanceof` will behave + * unpredictably. It is safest to avoid using `instanceof`, and using + * this type-testing method instead. + */ + public static isAssetManifestArtifact(art: any): art is AssetManifestArtifact { + return art && typeof art === 'object' && art[ASSET_MANIFEST_ARTIFACT_SYM]; + } + /** * The file name of the asset manifest */ @@ -36,3 +59,15 @@ export class AssetManifestArtifact extends CloudArtifact { this.bootstrapStackVersionSsmParameter = properties.bootstrapStackVersionSsmParameter; } } + +/** + * Mark all instances of 'AssetManifestArtifact' + * + * Why not put this in the constructor? Because this is a class property, + * not an instance property. It applies to all instances of the class. + */ +Object.defineProperty(AssetManifestArtifact.prototype, ASSET_MANIFEST_ARTIFACT_SYM, { + value: true, + enumerable: false, + writable: false, +}); diff --git a/packages/aws-cdk/lib/api/cloudformation-deployments.ts b/packages/aws-cdk/lib/api/cloudformation-deployments.ts index 60b883f13fb40..ebf40bbb7442b 100644 --- a/packages/aws-cdk/lib/api/cloudformation-deployments.ts +++ b/packages/aws-cdk/lib/api/cloudformation-deployments.ts @@ -405,7 +405,7 @@ export class CloudFormationDeployments { */ private async publishStackAssets(stack: cxapi.CloudFormationStackArtifact, toolkitInfo: ToolkitInfo) { const stackEnv = await this.sdkProvider.resolveEnvironment(stack.environment); - const assetArtifacts = stack.dependencies.filter(isAssetManifestArtifact); + const assetArtifacts = stack.dependencies.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact); for (const assetArtifact of assetArtifacts) { await this.validateBootstrapStackVersion( @@ -437,7 +437,3 @@ export class CloudFormationDeployments { } } } - -function isAssetManifestArtifact(art: cxapi.CloudArtifact): art is cxapi.AssetManifestArtifact { - return art instanceof cxapi.AssetManifestArtifact; -}