Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(cx-api): remove more instanceofs #19511

Merged
merged 2 commits into from
Mar 29, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
}