Skip to content

Commit 8f9d274

Browse files
authored
chore(cx-api): remove more instanceofs (#19511)
Fixes #17974 Follow-up of #14468 This follows the implementation of aws/constructs#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*
1 parent a0fb04f commit 8f9d274

File tree

4 files changed

+38
-15
lines changed

4 files changed

+38
-15
lines changed

packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts

+1-5
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ testFutureBehavior('nested assemblies share assets: default synth edition', flag
452452

453453
// Read the asset manifests to verify the file paths
454454
for (const stageName of ['Stage1', 'Stage2']) {
455-
const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(isAssetManifestArtifact)[0];
455+
const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact)[0];
456456
const manifest = JSON.parse(fs.readFileSync(manifestArtifact.file, { encoding: 'utf-8' }));
457457

458458
expect(manifest.dockerImages[DEMO_IMAGE_ASSET_HASH].source).toEqual({
@@ -464,7 +464,3 @@ testFutureBehavior('nested assemblies share assets: default synth edition', flag
464464
function isStackArtifact(x: any): x is cxapi.CloudFormationStackArtifact {
465465
return x instanceof cxapi.CloudFormationStackArtifact;
466466
}
467-
468-
function isAssetManifestArtifact(x: any): x is cxapi.AssetManifestArtifact {
469-
return x instanceof cxapi.AssetManifestArtifact;
470-
}

packages/@aws-cdk/aws-s3-assets/test/asset.test.ts

+1-5
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ test('nested assemblies share assets: default synth edition', () => {
275275

276276
// Read the asset manifests to verify the file paths
277277
for (const stageName of ['Stage1', 'Stage2']) {
278-
const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(isAssetManifestArtifact)[0];
278+
const manifestArtifact = assembly.getNestedAssembly(`assembly-${stageName}`).artifacts.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact)[0];
279279
const manifest = JSON.parse(fs.readFileSync(manifestArtifact.file, { encoding: 'utf-8' }));
280280

281281
expect(manifest.files[SAMPLE_ASSET_HASH].source).toEqual({
@@ -410,7 +410,3 @@ function mkdtempSync() {
410410
function isStackArtifact(x: any): x is cxapi.CloudFormationStackArtifact {
411411
return x instanceof cxapi.CloudFormationStackArtifact;
412412
}
413-
414-
function isAssetManifestArtifact(x: any): x is cxapi.AssetManifestArtifact {
415-
return x instanceof cxapi.AssetManifestArtifact;
416-
}

packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts

+35
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,33 @@ import * as cxschema from '@aws-cdk/cloud-assembly-schema';
33
import { CloudArtifact } from '../cloud-artifact';
44
import { CloudAssembly } from '../cloud-assembly';
55

6+
const ASSET_MANIFEST_ARTIFACT_SYM = Symbol.for('@aws-cdk/cx-api.AssetManifestArtifact');
7+
68
/**
79
* Asset manifest is a description of a set of assets which need to be built and published
810
*/
911
export class AssetManifestArtifact extends CloudArtifact {
12+
/**
13+
* Checks if `art` is an instance of this class.
14+
*
15+
* Use this method instead of `instanceof` to properly detect `AssetManifestArtifact`
16+
* instances, even when the construct library is symlinked.
17+
*
18+
* Explanation: in JavaScript, multiple copies of the `cx-api` library on
19+
* disk are seen as independent, completely different libraries. As a
20+
* consequence, the class `AssetManifestArtifact` in each copy of the `cx-api` library
21+
* is seen as a different class, and an instance of one class will not test as
22+
* `instanceof` the other class. `npm install` will not create installations
23+
* like this, but users may manually symlink construct libraries together or
24+
* use a monorepo tool: in those cases, multiple copies of the `cx-api`
25+
* library can be accidentally installed, and `instanceof` will behave
26+
* unpredictably. It is safest to avoid using `instanceof`, and using
27+
* this type-testing method instead.
28+
*/
29+
public static isAssetManifestArtifact(art: any): art is AssetManifestArtifact {
30+
return art && typeof art === 'object' && art[ASSET_MANIFEST_ARTIFACT_SYM];
31+
}
32+
1033
/**
1134
* The file name of the asset manifest
1235
*/
@@ -36,3 +59,15 @@ export class AssetManifestArtifact extends CloudArtifact {
3659
this.bootstrapStackVersionSsmParameter = properties.bootstrapStackVersionSsmParameter;
3760
}
3861
}
62+
63+
/**
64+
* Mark all instances of 'AssetManifestArtifact'
65+
*
66+
* Why not put this in the constructor? Because this is a class property,
67+
* not an instance property. It applies to all instances of the class.
68+
*/
69+
Object.defineProperty(AssetManifestArtifact.prototype, ASSET_MANIFEST_ARTIFACT_SYM, {
70+
value: true,
71+
enumerable: false,
72+
writable: false,
73+
});

packages/aws-cdk/lib/api/cloudformation-deployments.ts

+1-5
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ export class CloudFormationDeployments {
405405
*/
406406
private async publishStackAssets(stack: cxapi.CloudFormationStackArtifact, toolkitInfo: ToolkitInfo) {
407407
const stackEnv = await this.sdkProvider.resolveEnvironment(stack.environment);
408-
const assetArtifacts = stack.dependencies.filter(isAssetManifestArtifact);
408+
const assetArtifacts = stack.dependencies.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact);
409409

410410
for (const assetArtifact of assetArtifacts) {
411411
await this.validateBootstrapStackVersion(
@@ -437,7 +437,3 @@ export class CloudFormationDeployments {
437437
}
438438
}
439439
}
440-
441-
function isAssetManifestArtifact(art: cxapi.CloudArtifact): art is cxapi.AssetManifestArtifact {
442-
return art instanceof cxapi.AssetManifestArtifact;
443-
}

0 commit comments

Comments
 (0)