feat(core): present reason for cyclic references #2061

rix0rrr · 2019-03-20T14:26:17Z

To help people debugging cyclic references, we now trace the "reason"
a cyclic reference got added, so that we can present the conflicting
references instead of just presenting an error. Fix the order of
the error message, which was the wrong way around.

Clean up references a little:

Split out Reference and CfnReference, which got conflated in an
undesirable way. Reference is now officially the base class for all
references, and CfnReference is only one implementation of it for
CloudFormation references.
Make 'scope' required for references, the only place where it was
potentially empty was for CFN Pseudo Parameters, refactored those to
not use classes anymore (because there's no need to).
Get rid of 'Ref', the class wasn't being very useful.
Make a dependency Construct => Stack lazy (it was conflicting at
load time with the Stack => Construct dependency which is more
important).

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

To help people debugging cyclic references, we now trace the "reason" a cyclic reference got added, so that we can present the conflicting references instead of just presenting an error. Fix the order of the error message, which was the wrong way around. Clean up references a little: - Split out `Reference` and `CfnReference`, which got conflated in an undesirable way. `Reference` is now officially the base class for all references, and `CfnReference` is only one implementation of it for CloudFormation references. - Make 'scope' required for references, the only place where it was potentially empty was for CFN Pseudo Parameters, refactored those to not use classes anymore (because there's no need to). - Get rid of 'Ref', the class wasn't being very useful. - Make a dependency Construct => Stack lazy (it was conflicting at load time with the Stack => Construct dependency which is more important).

sam-goodwin · 2019-03-22T23:09:06Z

packages/@aws-cdk/cdk/lib/cfn-reference.ts

+   * Check whether this is actually a Reference
+   */
+  public static isCfnReferenceToken(x: Token): x is CfnReference {
+    return (x as any).consumeFromStack !== undefined;


I think you could instead use a private symbol to make this 100% reliable:

// don't export const isReference = Symbol.for('CfnReference'); export class CfnReference extends Reference { // private shouldn't bother JSII private [isReference] = true; public static isCfnReferenceToken(x: Token): x is CfnReference { return (x as any)[isReference] === true; } }

private doesn't bother JSII but it does bother TypeScript, which complains about unused private members.

I am also not 100% sure that a private symbol will work across two different versions of this module (which is the reason we can't use instanceof).

My favorite (and very ugly still) way to do this is to add the marker at runtime:

const MARKER_KEY = '$type-marker'; const MARKER = '<uuid>'; export class MyClass { public static isMyClass(obj: any): obj is MyClass { return obj[MARKER_KEY] === MARKER; } constructor() { Object.defineProperty(this, MARKER_KEY, { value: MARKER, enumerable: false }); } }

Yeah, horrible. TypeScript, can you make this easier?

It will work across two different versions because Symbol.for('key') stores the symbol in the global symbol registry by name. It would not work if you were to just call Symbol(), which is unique for every call. By private, I mean hiding it from JSII.

Your UUID approach is an emulation of the global Symbol registry which was designed to solve this problem.

We can use this to avoid some of the ugly duck typing we do - mark up the core constructs with symbols and maybe even generate some for L1. Module augmentation may even allow us to generate for our L2 later.

Create a symbol for each type that would be checked by name in a nominal type system.

Yes, Symbol.for() will fix it. Although I do wonder what the point of making it a Symbol is, then, since we lose the uniqueness value of it. I guess that it can't conflict with a naive string.

On top of avoiding accidental conflicts, Symbols are not considered properties so it won't show up when enumerating an object's keys. Basically, it's cleaner and simpler.

eladb

Lots of goodness!

eladb · 2019-03-24T11:25:20Z

packages/@aws-cdk/cdk/lib/cfn-reference.ts

+   * Check whether this is actually a Reference
+   */
+  public static isCfnReferenceToken(x: Token): x is CfnReference {
+    return (x as any).consumeFromStack !== undefined;


I am also not 100% sure that a private symbol will work across two different versions of this module (which is the reason we can't use instanceof).

My favorite (and very ugly still) way to do this is to add the marker at runtime:

const MARKER_KEY = '$type-marker'; const MARKER = '<uuid>'; export class MyClass { public static isMyClass(obj: any): obj is MyClass { return obj[MARKER_KEY] === MARKER; } constructor() { Object.defineProperty(this, MARKER_KEY, { value: MARKER, enumerable: false }); } }

Yeah, horrible. TypeScript, can you make this easier?

eladb · 2019-03-24T11:27:31Z

packages/@aws-cdk/cdk/lib/pseudo.ts

 import { Token } from './token';

+const AWS_ACCOUNTID = 'AWS::AccountId';


Why are these consts needed? Aren't they used exactly once?

Once for the scoped, once for the unscoped variant.

eladb · 2019-03-24T11:28:36Z

packages/@aws-cdk/cdk/lib/reference.ts

- * This magic happens in the prepare() phase, where consuming stacks will call
- * `consumeFromStack` on these Tokens and if they happen to be exported by a different
- * Stack, we'll register the dependency.
+ * References are recorded.
 */
 export class Reference extends Token {
  /**
   * Check whether this is actually a Reference
   */
  public static isReferenceToken(x: Token): x is Reference {


isReference?

I'm all for it, but the previous implementor (you?) picked this name.

eladb · 2019-03-24T11:29:25Z

packages/@aws-cdk/cdk/lib/reference.ts

-    if (displayName && scope) {
-      displayName = `${scope.node.path}.${displayName}`;
-    }
+  constructor(value: any, displayName: string, target: Construct) {


I still feel we could automatically let Reference tokens know that they are being used instead of explicit consumeFromStack

To me, that's the essence of a reference token. Then CfnReference utilizes this capability to do the cross-stack magic, but the behavior is polymorphic. Otherwise, what's the added value in actually modeling the concept of a reference?

rix0rrr requested a review from a team as a code owner March 20, 2019 14:26

rix0rrr added 2 commits March 20, 2019 15:32

Satisfy jsii by exporting

de68555

Get rid of some trailing AwsNoValue() references

63cbcdf

rix0rrr requested review from RomainMuller and skinny85 as code owners March 20, 2019 14:48

Rico Huijbers added 2 commits March 20, 2019 19:31

Another ScopedAws that I missed

04a18f4

And 2 more

3db479b

rix0rrr self-assigned this Mar 21, 2019

sam-goodwin reviewed Mar 22, 2019

View reviewed changes

eladb approved these changes Mar 24, 2019

View reviewed changes

rix0rrr added 2 commits March 26, 2019 14:15

Use Symbol to test for class type

ad48bcf

Also use a symbol for Reference

b233cf9

rix0rrr merged commit e82e208 into master Mar 26, 2019

rix0rrr deleted the huijbers/cyclic-ref-reasons branch March 26, 2019 14:07

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

This was referenced Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.22.0 to 0.35.0 MechanicalRock/scdk#3

Closed

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/cdk-constructs#8

Open

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/account-reaper#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): present reason for cyclic references #2061

feat(core): present reason for cyclic references #2061

rix0rrr commented Mar 20, 2019

sam-goodwin Mar 22, 2019

rix0rrr Mar 23, 2019

eladb Mar 24, 2019

sam-goodwin Mar 25, 2019 •

edited

Loading

sam-goodwin Mar 25, 2019

rix0rrr Mar 25, 2019 •

edited

Loading

sam-goodwin Mar 25, 2019

eladb left a comment

eladb Mar 24, 2019

eladb Mar 24, 2019

rix0rrr Mar 25, 2019

eladb Mar 24, 2019

rix0rrr Mar 25, 2019

eladb Mar 24, 2019

eladb Mar 24, 2019

		import { Token } from './token';

		const AWS_ACCOUNTID = 'AWS::AccountId';

feat(core): present reason for cyclic references #2061

feat(core): present reason for cyclic references #2061

Conversation

rix0rrr commented Mar 20, 2019

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-goodwin Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-goodwin Mar 25, 2019 •

edited

Loading

rix0rrr Mar 25, 2019 •

edited

Loading