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

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

Merged
merged 7 commits into from
Mar 26, 2019

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Mar 20, 2019

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).
@rix0rrr rix0rrr requested a review from a team as a code owner March 20, 2019 14:26
@rix0rrr rix0rrr self-assigned this Mar 21, 2019
* Check whether this is actually a Reference
*/
public static isCfnReferenceToken(x: Token): x is CfnReference {
return (x as any).consumeFromStack !== undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@sam-goodwin sam-goodwin Mar 25, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rix0rrr rix0rrr Mar 25, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Lots of goodness!

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

Choose a reason for hiding this comment

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

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?

import { Token } from './token';

const AWS_ACCOUNTID = 'AWS::AccountId';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once for the scoped, once for the unscoped variant.

* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

isReference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants