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.

Sign up for GitHub

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

Jump to bottom

chore(core): isCfnResource allows any type as input #28001

Merged
merged 4 commits into from
Nov 29, 2023
Merged

chore(core): isCfnResource allows any type as input #28001

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f573d6a
chore: isCfnResource allows any type as input
tmokmss Nov 15, 2023
3599864
Update cfn-resource.ts
tmokmss Nov 15, 2023
e3c96ec
Merge branch 'main' into is_cfnresource_allow_undefined
vinayak-kukreja Nov 29, 2023
2a973e3
Merge branch 'main' into is_cfnresource_allow_undefined
mergify[bot] Nov 29, 2023
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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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);
});
});