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(core): isCfnResource allows any type as input #28001

Merged
merged 4 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
8 changes: 4 additions & 4 deletions packages/aws-cdk-lib/core/lib/cfn-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CfnCondition } from './cfn-condition';
/* eslint-disable import/order */
import { CfnRefElement } from './cfn-element';
import { CfnCreationPolicy, CfnDeletionPolicy, CfnUpdatePolicy } from './cfn-resource-policy';
import { Construct, IConstruct, Node } from 'constructs';
import { Construct, Node } from 'constructs';
import { addDependency, obtainDependencies, removeDependency } from './deps';
import { CfnReference } from './private/cfn-reference';
import { Reference } from './reference';
Expand Down Expand Up @@ -34,10 +34,10 @@ export interface CfnResourceProps {
*/
export class CfnResource extends CfnRefElement {
/**
* Check whether the given construct is a CfnResource
* Check whether the given object is a CfnResource
*/
public static isCfnResource(construct: IConstruct): construct is CfnResource {
return (construct as any).cfnResourceType !== undefined;
public static isCfnResource(x: any): x is CfnResource {
return x !== null && typeof(x) === 'object' && x.cfnResourceType !== undefined;
Copy link
Contributor

@go-to-k go-to-k Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other similar isXxx methods check by Symbol, is there any reason not to do the following? I thought it would work as it is more safe and also keeps the consistent signatures.

Because the cfnResourceType can be considered unique property?

const CFN_RESOURCE_SYMBOL = Symbol.for('@aws-cdk/core.CfnResource');
// ...
  public static isCfnResource(x: any): x is CfnResource {
    return x !== null && typeof(x) === 'object' && CFN_RESOURCE_SYMBOL in x;
  }
  // ...
  constructor(scope: Construct, id: string, props: CfnResourceProps) {
    // ...
    Object.defineProperty(this, CFN_RESOURCE_SYMBOL, { value: true });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'd like to keep this as it is to prevent from possible breaking change unless there is a strong reason to do so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks!

}

// MAINTAINERS NOTE: this class serves as the base class for the generated L1
Expand Down
24 changes: 24 additions & 0 deletions packages/aws-cdk-lib/core/test/cfn-resource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -412,4 +412,28 @@ describe('cfn resource', () => {
delete process.env.CDK_DEBUG;
}
});

test('isCfnResource returns true with a CfnResource', () => {
const app = new core.App();
const stack = new core.Stack(app, 'Stack');
const res = new core.CfnResource(stack, 'SomeCfnResource', {
type: 'Some::Resource',
});

// THEN
expect(core.CfnResource.isCfnResource(res)).toBe(true);
});

test('isCfnResource returns false with a construct', () => {
const app = new core.App();
const stack = new core.Stack(app, 'Stack');

// THEN
expect(core.CfnResource.isCfnResource(stack)).toBe(false);
});

test('isCfnResource returns false with undefined', () => {
// THEN
expect(core.CfnResource.isCfnResource(undefined)).toBe(false);
});
});