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

feat(core): warn if an aspect was added via another aspect #8639

Merged
merged 7 commits into from
Aug 3, 2020
Merged

feat(core): warn if an aspect was added via another aspect #8639

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f486d7f
chore(core): warn if an aspect was added via another aspect
Jun 18, 2020
333cea9
Merge branch 'master' into neta/warn-nested-aspects
Jun 21, 2020
0eb9073
temp
Jun 22, 2020
1eddf65
Merge branch 'master' into neta/warn-nested-aspects
NetaNir Jul 28, 2020
6845ce0
add unit tests
NetaNir Jul 28, 2020
cca6ce7
Merge branch 'master' into neta/warn-nested-aspects
NetaNir Jul 28, 2020
8e264a7
Merge branch 'master' into neta/warn-nested-aspects
mergify[bot] Aug 3, 2020
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
10 changes: 9 additions & 1 deletion packages/@aws-cdk/core/lib/private/synthesis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,25 @@ function synthNestedAssemblies(root: IConstruct, options: StageSynthesisOptions)
* twice for the same construct.
*/
function invokeAspects(root: IConstruct) {
let nestedAspectWarning = false;
recurse(root, []);

function recurse(construct: IConstruct, inheritedAspects: constructs.IAspect[]) {
// hackery to be able to access some private members with strong types (yack!)
const node: NodeWithAspectPrivatesHangingOut = construct.node._actualNode as any;

const allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];

const nodeAspectsCount = node._aspects.length;
for (const aspect of allAspectsHere) {
if (node.invokedAspects.includes(aspect)) { continue; }
aspect.visit(construct);
// if an aspect was added to the node while invoking another aspect it will not be invoked, emit a warning
// the `nestedAspectWarning` flag is used to prevent the warning from being emitted for every child
if (!nestedAspectWarning && nodeAspectsCount !== node._aspects.length) {
// tslint:disable-next-line: max-line-length
construct.node.addWarning('We detected an aspect was added via another aspect, this is not supported and may result in an unexpected behavior');
Copy link
Contributor

@rix0rrr rix0rrr Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We can be more specific than the vague "may result in unexpected behavior."

How about something like?

Aspects added by other Aspects will not be applied; add all Aspects before 'synth' is called.

Copy link
Contributor

@rix0rrr rix0rrr Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If we wanted to go with restoring the original behavior, it would look something like this:

const appliedHere = new Set<IAspect>();
let allAspectsHere: IAspect[];
while (true) {
  allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];
  const yetToApply = allAspectsHere.filter(x => !appliedHere.has(x));
  if (yetToApply.length === 0) { break; }
  
  for (const aspect of yetToApply) { 
    // ...
    appliedHere.add(aspect);
  }
}

recurse(child, allAspectsHere);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eladb @rix0rrr I think we should restore the previous behavior, if only to maintain backward compatibility, if no objection I'll create the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

No objections

Copy link
Contributor

Choose a reason for hiding this comment

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

? Any updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Any updates?

nestedAspectWarning = true;
}
node.invokedAspects.push(aspect);
}

Expand Down