Skip to content

Commit

Permalink
chore(cx-api): break circular dependencies (#18767)
Browse files Browse the repository at this point in the history
Use the "Declaration Merging" feature from TypeScript, combined
with the "Monkey Patching" feature from JavaScript, to break the
circular dependencies between some of the classes in this library.

These dependency cycles interfere with proper bundling by `esbuild`.

Use `madge` to validate that we don't add those cycles back. Madge
validation only happens on the `.js` files, because those are the only
ones we care about when bundling. Cyclic imports that are only used to
import type information won't actually cause `require()` cycles at
runtime, since types don't exist in JS.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Feb 3, 2022
1 parent be0be01 commit ec8495a
Show file tree
Hide file tree
Showing 8 changed files with 565 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as fs from 'fs';
import * as path from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { CloudArtifact } from '../cloud-artifact';
import { CloudAssembly } from '../cloud-assembly';
import type { CloudAssembly } from '../cloud-assembly';
import { Environment, EnvironmentUtils } from '../environment';

export class CloudFormationStackArtifact extends CloudArtifact {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { CloudAssembly } from '../cloud-assembly';
import { NestedCloudAssemblyArtifact } from './nested-cloud-assembly-artifact';

const cacheSym = Symbol();

/**
* The nested Assembly
*
* Declared in a different file to break circular dep between CloudAssembly and NestedCloudAssemblyArtifact
*/
Object.defineProperty(NestedCloudAssemblyArtifact.prototype, 'nestedAssembly', {
get() {
if (!this[cacheSym]) {
this[cacheSym] = new CloudAssembly(this.fullPath);
}
return this[cacheSym];
},
});
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as path from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { CloudArtifact } from '../cloud-artifact';
import { CloudAssembly } from '../cloud-assembly';
import type { CloudAssembly } from '../cloud-assembly';

/**
* Asset manifest is a description of a set of assets which need to be built and published
Expand All @@ -17,11 +17,6 @@ export class NestedCloudAssemblyArtifact extends CloudArtifact {
*/
public readonly displayName: string;

/**
* Cache for the inner assembly loading
*/
private _nestedAssembly?: CloudAssembly;

constructor(assembly: CloudAssembly, name: string, artifact: cxschema.ArtifactManifest) {
super(assembly, name, artifact);

Expand All @@ -36,14 +31,13 @@ export class NestedCloudAssemblyArtifact extends CloudArtifact {
public get fullPath(): string {
return path.join(this.assembly.directory, this.directoryName);
}
}

export interface NestedCloudAssemblyArtifact {
/**
* The nested Assembly
*/
public get nestedAssembly(): CloudAssembly {
if (!this._nestedAssembly) {
this._nestedAssembly = new CloudAssembly(this.fullPath);
}
return this._nestedAssembly;
}
}
readonly nestedAssembly: CloudAssembly;

// Declared in a different file
}
32 changes: 32 additions & 0 deletions packages/@aws-cdk/cx-api/lib/cloud-artifact-aug.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { AssetManifestArtifact } from './artifacts/asset-manifest-artifact';
import { CloudFormationStackArtifact } from './artifacts/cloudformation-artifact';
import { NestedCloudAssemblyArtifact } from './artifacts/nested-cloud-assembly-artifact';
import { TreeCloudArtifact } from './artifacts/tree-cloud-artifact';
import { CloudArtifact } from './cloud-artifact';
import { CloudAssembly } from './cloud-assembly';

/**
* Add the 'fromManifest' factory function
*
* It is defined in a separate file to avoid circular dependencies between 'cloud-artifact.ts'
* and all of its subclass files.
*/
CloudArtifact.fromManifest = function fromManifest(
assembly: CloudAssembly,
id: string,
artifact: cxschema.ArtifactManifest,
): CloudArtifact | undefined {
switch (artifact.type) {
case cxschema.ArtifactType.AWS_CLOUDFORMATION_STACK:
return new CloudFormationStackArtifact(assembly, id, artifact);
case cxschema.ArtifactType.CDK_TREE:
return new TreeCloudArtifact(assembly, id, artifact);
case cxschema.ArtifactType.ASSET_MANIFEST:
return new AssetManifestArtifact(assembly, id, artifact);
case cxschema.ArtifactType.NESTED_CLOUD_ASSEMBLY:
return new NestedCloudAssemblyArtifact(assembly, id, artifact);
default:
return undefined;
}
};
24 changes: 5 additions & 19 deletions packages/@aws-cdk/cx-api/lib/cloud-artifact.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { CloudAssembly } from './cloud-assembly';
import type { CloudAssembly } from './cloud-assembly';
import { MetadataEntryResult, SynthesisMessage, SynthesisMessageLevel } from './metadata';

/**
Expand Down Expand Up @@ -36,24 +36,16 @@ export interface AwsCloudFormationStackProperties {
export class CloudArtifact {
/**
* Returns a subclass of `CloudArtifact` based on the artifact type defined in the artifact manifest.
*
* @param assembly The cloud assembly from which to load the artifact
* @param id The artifact ID
* @param artifact The artifact manifest
* @returns the `CloudArtifact` that matches the artifact type or `undefined` if it's an artifact type that is unrecognized by this module.
*/
public static fromManifest(assembly: CloudAssembly, id: string, artifact: cxschema.ArtifactManifest): CloudArtifact | undefined {
switch (artifact.type) {
case cxschema.ArtifactType.AWS_CLOUDFORMATION_STACK:
return new CloudFormationStackArtifact(assembly, id, artifact);
case cxschema.ArtifactType.CDK_TREE:
return new TreeCloudArtifact(assembly, id, artifact);
case cxschema.ArtifactType.ASSET_MANIFEST:
return new AssetManifestArtifact(assembly, id, artifact);
case cxschema.ArtifactType.NESTED_CLOUD_ASSEMBLY:
return new NestedCloudAssemblyArtifact(assembly, id, artifact);
default:
return undefined;
}
// Implementation is defined in a separate file to break cyclic dependencies
void(assembly), void(id), void(artifact);
throw new Error('Implementation not overridden yet');
}

/**
Expand Down Expand Up @@ -152,9 +144,3 @@ export class CloudArtifact {
return this.manifest.displayName ?? this.id;
}
}

// needs to be defined at the end to avoid a cyclic dependency
import { AssetManifestArtifact } from './artifacts/asset-manifest-artifact';
import { CloudFormationStackArtifact } from './artifacts/cloudformation-artifact';
import { NestedCloudAssemblyArtifact } from './artifacts/nested-cloud-assembly-artifact';
import { TreeCloudArtifact } from './artifacts/tree-cloud-artifact';
2 changes: 2 additions & 0 deletions packages/@aws-cdk/cx-api/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ export * from './context/endpoint-service-availability-zones';
export * from './context/security-group';
export * from './context/key';
export * from './cloud-artifact';
import './cloud-artifact-aug';
export * from './artifacts/asset-manifest-artifact';
export * from './artifacts/cloudformation-artifact';
export * from './artifacts/tree-cloud-artifact';
export * from './artifacts/nested-cloud-assembly-artifact';
import './artifacts/nested-cloud-assembly-artifact-aug';
export * from './cloud-assembly';
export * from './assets';
export * from './environment';
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/cx-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"scripts": {
"build": "cdk-build",
"watch": "cdk-watch",
"lint": "cdk-lint",
"lint": "cdk-lint && madge --circular --extensions js lib",
"test": "cdk-test",
"pkglint": "pkglint -f",
"package": "cdk-package",
Expand Down Expand Up @@ -72,6 +72,7 @@
"@types/mock-fs": "^4.13.1",
"@types/semver": "^7.3.9",
"jest": "^27.4.7",
"madge": "^5.0.1",
"mock-fs": "^4.14.0"
},
"repository": {
Expand Down
Loading

0 comments on commit ec8495a

Please sign in to comment.