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

(aws-core): Aspects are not visited if added via module #18921

Closed
joshskinner-vertex opened this issue Feb 10, 2022 · 2 comments · Fixed by #19491
Closed

(aws-core): Aspects are not visited if added via module #18921

joshskinner-vertex opened this issue Feb 10, 2022 · 2 comments · Fixed by #19491
Labels
bug This issue is a bug. p1 package/tools Related to AWS CDK Tools or CLI

Comments

@joshskinner-vertex
Copy link

What is the problem?

We have been migrating to CDK v2 and noticed our Aspects were not visited during a cdk synth. Tracking it down, we found that Aspects are visited if they are added in the app code but remain unvisited if added in library code.

Reproduction Steps

Overview of Steps

  • create mylib and myapp projects in each CDK Framework version
  • see expected output in a CDK v1 (1.116.0) project set
  • see unexpected output in a migrated CDK v2 (2.12.0) project set

myapp (v1)

./v1/myapp/bin/myapp.ts

import 'source-map-support/register';
import * as cdk from '@aws-cdk/core';
import { MyStack } from '../lib/myapp-stack';

const app = new cdk.App();
new MyStack(app);

./v1/myapp/lib/myapp-stack.tst

import * as cdk from '@aws-cdk/core';
import {
  MyAspect, MyLibStack
} from "mylib";

export class MyStack extends MyLibStack {
  constructor(scope: cdk.Construct) {
    super(scope);
    cdk.Aspects.of(this).add(new MyAspect(this, 'MyStack'));
  }
}

mylib (v1)

./v1/mylib/lib/index.ts

import * as cdk from '@aws-cdk/core';

export class MyLibStack extends cdk.Stack {
  constructor(scope: cdk.Construct) {
    super(scope);
    cdk.Aspects.of(this).add(new MyAspect(this, 'MyLibStack'));
  }
}

export class MyAspect implements cdk.IAspect {
    private readonly whoami: string;

    public constructor(scope: cdk.Construct, whoami: string) {
      this.whoami = whoami;
      console.log(this.whoami + ' MyAspect constructor: ' + scope.node.path);
    }
  
    public visit(node: cdk.IConstruct): void {
      console.log(this.whoami + ' visit: ' + node.node.path);
    }
}

Expected output of cdk synth -q:

MyLibStack MyAspect constructor: Default
MyStack MyAspect constructor: Default
MyLibStack visit: Default
MyStack visit: Default

myapp (v2)

./v2/myapp/bin/myapp.ts

import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import { MyStack } from '../lib/myapp-stack';

const app = new cdk.App();
new MyStack(app);

./v2/myapp/lib/myapp-stack.tst

import * as cdk from 'aws-cdk-lib';
import {
  MyAspect, MyLibStack
} from "mylib";
import { Construct } from 'constructs';

export class MyStack extends MyLibStack {
  constructor(scope: Construct) {
    super(scope);
    cdk.Aspects.of(this).add(new MyAspect(this, 'MyStack'));
  }
}

mylib (v2)

./v2/mylib/lib/index.ts

import * as cdk from 'aws-cdk-lib';
import { Construct, IConstruct } from 'constructs';

export class MyLibStack extends cdk.Stack {
  constructor(scope: Construct) {
    super(scope);
    cdk.Aspects.of(this).add(new MyAspect(this, 'MyLibStack'));
  }
}

export class MyAspect implements cdk.IAspect {
    private readonly whoami: string;

    public constructor(scope: Construct, whoami: string) {
      this.whoami = whoami;
      console.log(this.whoami + ' MyAspect constructor: ' + scope.node.path);
    }
  
    public visit(node: IConstruct): void {
      console.log(this.whoami + ' visit: ' + node.node.path);
    }
}

Unexpected output of cdk synth -q:

MyLibStack MyAspect constructor: Default
MyStack MyAspect constructor: Default
MyStack visit: Default

What did you expect to happen?

Expected to see identical output from the cdk synth -q commands of the v1 and v2 apps.

For both projects, specifically,

MyLibStack MyAspect constructor: Default
MyStack MyAspect constructor: Default
MyLibStack visit: Default
MyStack visit: Default

What actually happened?

The following message was not seen.

MyLibStack visit: Default

Meaning, the Aspect added to the Stack in the library code was not visited during the cdk synth

CDK CLI Version

2.12.0 (build c9786db)

Framework Version

2

Node.js Version

v14.17.1

OS

Mac OSX 12.1

Language

Typescript

Language Version

~3.9.7

Other information

For these reproducible steps, I used cdk init lib --language typescript and cdk init app --language typescript to generate the both sets of projects. I then edited the v1 projects to use CDK v1 by changing the package.json dependencies, removing the node_modules folders, migrating the code itself, and running yarn build and link. I can provide the entire directory structure if it would help.

@joshskinner-vertex joshskinner-vertex added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 10, 2022
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Feb 10, 2022
@NGL321 NGL321 added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Feb 14, 2022
@RichiCoder1
Copy link
Contributor

I've also run into this. Worth noting too that I think this wasn't introduced with v2 but at some point later into v2's lifecycle. This was working after we migrated to v2, but broke at some point during a v2 upgrade.

rix0rrr added a commit that referenced this issue Mar 21, 2022
When construct libraries are purposely symlinked (as opposed of
collectively `npm install`ed), depending on how this is done they may
end up with multiple copies of `aws-cdk-lib`. If that happens, Aspects
from a different `aws-cdk-lib` copy than the one providing `App` will
not be applied.

The reason is the use of `Symbol('...')` instead of `Symbol.for('...')`
to keep the list of aspects on the construct object.

- The first version creates a unique symbol per library, while adding
  a naming hint.
- The second version deduplicates: all symbols with the same naming
  hint will receive the same symbol.

The second version is necessary to make sure that different copies
of the `aws-cdk-lib` library store their aspects under the same key.

Fixes #18921, #18778, #19390.
@mergify mergify bot closed this as completed in #19491 Mar 22, 2022
mergify bot pushed a commit that referenced this issue Mar 22, 2022
When construct libraries are purposely symlinked (as opposed of
collectively `npm install`ed), depending on how this is done they may
end up with multiple copies of `aws-cdk-lib`. If that happens, Aspects
from a different `aws-cdk-lib` copy than the one providing `App` will
not be applied.

The reason is the use of `Symbol('...')` instead of `Symbol.for('...')`
to keep the list of aspects on the construct object.

- The first version creates a unique symbol per library, while adding
  a naming hint.
- The second version deduplicates: all symbols with the same naming
  hint will receive the same symbol.

The second version is necessary to make sure that different copies
of the `aws-cdk-lib` library store their aspects under the same key.

Fixes #18921, #18778, #19390, #18914
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p1 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants