Skip to content

Commit

Permalink
chore(cx-api): remove more instanceofs (aws#19511)
Browse files Browse the repository at this point in the history
Fixes aws#17974

Follow-up of aws#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*
  • Loading branch information
mnapoli authored and Stephen Potter committed Apr 27, 2022
1 parent fac1b43 commit 88e6d86
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 15 deletions.
6 changes: 1 addition & 5 deletions packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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;
}
6 changes: 1 addition & 5 deletions packages/@aws-cdk/aws-s3-assets/test/asset.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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;
}
35 changes: 35 additions & 0 deletions packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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,
});
6 changes: 1 addition & 5 deletions packages/aws-cdk/lib/api/cloudformation-deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -437,7 +437,3 @@ export class CloudFormationDeployments {
}
}
}

function isAssetManifestArtifact(art: cxapi.CloudArtifact): art is cxapi.AssetManifestArtifact {
return art instanceof cxapi.AssetManifestArtifact;
}

0 comments on commit 88e6d86

Please sign in to comment.