Aws-cdk: [core] Multiple projects breaks CDK synth (using instanceof cross-project)

Created on 20 Oct 2020  Β·  7Comments  Β·  Source: aws/aws-cdk

:question: General Issue

Creating a Construct, shared by multiple stacks, in a separate project, fails to deploy in CDK version >= 1.57.0

In our setup we have multiple stacks sharing misc. Constructs. These Constructs are all in separate Typescript/NodeJS projects.
We use Lerna to maintain/link dependencies between projects.

A simplified example of the structure could be:

➜ tree
.
β”œβ”€β”€ README.md
β”œβ”€β”€ lerna.json
β”œβ”€β”€ package-lock.json
β”œβ”€β”€ package.json
└── packages
    β”œβ”€β”€ myapp
    β”‚Β Β  β”œβ”€β”€ README.md
    β”‚Β Β  β”œβ”€β”€ bin
    β”‚Β Β  β”‚Β Β  └── myapp.ts
    β”‚Β Β  β”œβ”€β”€ cdk.json
    β”‚Β Β  β”œβ”€β”€ jest.config.js
    β”‚Β Β  β”œβ”€β”€ lambda
    β”‚Β Β  β”‚Β Β  └── index.ts
    β”‚Β Β  β”œβ”€β”€ lib
    β”‚Β Β  β”‚Β Β  └── myapp-stack.ts
    β”‚Β Β  β”œβ”€β”€ package-lock.json
    β”‚Β Β  β”œβ”€β”€ package.json
    β”‚Β Β  β”œβ”€β”€ test
    β”‚Β Β  β”‚Β Β  └── myapp.test.ts
    β”‚Β Β  └── tsconfig.json
    └── myconstruct
        β”œβ”€β”€ README.md
        β”œβ”€β”€ jest.config.js
        β”œβ”€β”€ lambda
        β”‚Β Β  └── index.ts
        β”œβ”€β”€ lib
        β”‚Β Β  └── index.ts
        β”œβ”€β”€ package-lock.json
        β”œβ”€β”€ package.json
        β”œβ”€β”€ test
        β”‚Β Β  └── myconstruct.test.ts
        └── tsconfig.json

(an example can be found here: https://github.com/gwriss/upgrade_bug)

Running

➜ npx lerna bootstrap

will npm-install all dependencies and link projects like this (snipped for better overview):

➜ tree -L 4
.
β”œβ”€β”€ ...
└── packages
    β”œβ”€β”€ myapp
    β”‚Β Β  β”œβ”€β”€ ...
    β”‚Β Β  β”œβ”€β”€ node_modules
    β”‚Β Β  β”‚Β Β  β”œβ”€β”€ ...
    β”‚Β Β  β”‚Β Β  β”œβ”€β”€ @aws-cdk
    β”‚Β Β  β”‚Β Β  β”œβ”€β”€ ...
    β”‚Β Β  β”‚Β Β  β”œβ”€β”€ myconstruct -> ../../myconstruct
    β”‚Β Β  β”‚Β Β  └── ...
    β”‚Β Β  └── ...
    └── myconstruct
        β”œβ”€β”€ ...
        β”œβ”€β”€ node_modules
        β”‚Β Β  β”œβ”€β”€ ...
        β”‚Β Β  β”œβ”€β”€ @aws-cdk
        β”‚Β Β  └── ...
        └── ...

Now, when the Construct in myconstruct contains a Lambda Function and the Myapp in myapp instantiate an instance of Myconstruct, the synth of the app fails:

➜ npx lerna run build
lerna notice cli v3.22.1
...
...
lerna success - myapp
lerna success - myconstruct
➜ cd packages/myapp
➜ npx cdk synth
unable to determine cloud assembly output directory. Assets must be defined indirectly within a "Stage" or an "App" scope
Subprocess exited with error 1

(an example of the setup can be found and tested here: https://github.com/gwriss/upgrade_bug)

Diving into the problem, it seems like it's another issue with instanceof comparing two instances of the same class, but from different node_modules.

I think @tneely was onto something in https://github.com/aws/aws-cdk/issues/9546#issuecomment-671064704, but decided to close the issue without a resolution.

Another issued related to this is the "Duck Typing" fix of Stack in https://github.com/aws/aws-cdk/issues/10671.

⚠️ Notice
Please don't pollute the discussion with suggestions about "deleting node_modules & reinstall" and "checking modules are all of the same version".
That is another issue but has nothing to do with this issue!

The Question

Did I miss som crucial point in our preferred project setup? I would really like a discussion about the recommended way of organising bigger CDK apps/projects. This could hopefully lead to a new topic in developer guide.

@rix0rrr suggest a peerDependency solution for a similar problem here: https://github.com/aws/aws-cdk/commit/5e10b94aaaa87e6479e0bfcbe844d9fa68d0c1f5
but I fail to see how this would solve the problem. (please enlighten me if I missed something πŸ˜ƒ )

At the moment this is a show-stopper for our long-overdue upgrade of CDK

Reference

Environment

  • CDK CLI Version: 1.68.0 (build a6a3f46)
  • Module Version:
  • Node.js Version: v12.19.0
  • OS:
  • Language (Version): TypeScript 3.9.7

Other information

@aws-cdcore guidance needs-triage

Most helpful comment

https://github.com/aws/aws-cdk/pull/11113 will most likely fix this issue (still needs to be confirmed when CDK 1.72.0 is released)

All 7 comments

BTW. I tried to fix this issue by replacing instanceof with "Duck Typing" in stage.ts

diff --git a/packages/@aws-cdk/core/lib/stage.ts b/packages/@aws-cdk/core/lib/stage.ts
index 50ef0b7c5..7e110ba40 100644
--- a/packages/@aws-cdk/core/lib/stage.ts
+++ b/packages/@aws-cdk/core/lib/stage.ts
@@ -7,6 +7,8 @@ import { synthesize } from './private/synthesis';
 // eslint-disable-next-line
 import { Construct as CoreConstruct } from './construct-compat';

+const STAGE_SYMBOL = Symbol.for('@aws-cdk/core.Stage');
+
 /**
  * Initialization props for a stage.
  */
@@ -84,8 +86,12 @@ export class Stage extends CoreConstruct {
    *
    * @experimental
    */
-  public static isStage(x: any ): x is Stage {
-    return x !== null && x instanceof Stage;
+  public static isStage(x: any): x is Stage {
+    // Original check
+    // return x !== null && x instanceof Stage;
+
+    // Duck typing check copied from stack.ts:153 isStack(x: any)
+    return x !== null && typeof (x) === 'object' && STAGE_SYMBOL in x;
   }

   /**

But that kind of blew up the tests in core

Test result after duck typing fix

@aws-cdk/core: Test Suites: 35 failed, 14 passed, 49 total
@aws-cdk/core: Tests:       300 failed, 249 passed, 549 total
@aws-cdk/core: Snapshots:   0 total
@aws-cdk/core: Time:        10.622 s
@aws-cdk/core: Ran all test suites.

Maybe someone with more in-depth experience with the core package can see what is going on.

This seems to be the same problem but the issue is closed: https://github.com/aws/aws-cdk/issues/10210#issuecomment-690093916

I think this just flat-out won't work, and Lerna isn't smart enough to do it.

Yarn will hoist the modules appropriately. Otherwise you're going to have to declare a dependency on CDK in devDependencies at the top-level of your monorepo.

I understand that CDK, as a project, does not want to fix the dependency problems with npm/lerna. Totally understandable.

Still, this will most likely become a problem lots of people will face in the future, when their CDK projects gets bigger and they need to rearrange their source code into separate projects.

@rix0rrr would it be interesting to have a section in the developer guide for organising bigger CDK projects? What is the best way to proceed with this?

https://github.com/aws/aws-cdk/pull/11113 will most likely fix this issue (still needs to be confirmed when CDK 1.72.0 is released)

I think this just flat-out won't work, and Lerna isn't smart enough to do it.

Yarn will hoist the modules appropriately. Otherwise you're going to have to declare a dependency on CDK in devDependencies at the top-level of your monorepo.

For future reference, when people face this issue:

I created a new branch use_lerna_yarn_workspaces testing @rix0rrr suggestion using yarn hoisting.

It works... but please read this before choosing this path: https://rushjs.io/pages/advanced/phantom_deps/

For our project we chose another path, using Rush instead of lerna/yarn. The switch is not a not small task for a big project, but the dependency handling solutions seems so much better, we figured it was worth it. See this branch: try_with_rush for a minimal example of what is required and that it actually works.

Note ⚠️
Rush is very opinionated but will solve the dependency problems we are facing.
For fixing all dependency problems (read: https://rushjs.io/pages/advanced/npm_doppelgangers/) we also need to switch to pnpm, which is a even bigger task, that probably will introduce new problems (https://rushjs.io/pages/maintainer/package_managers/). We postponed that for now, and are still using npm.

I will close this issue because I don't expect any solution to be found within the CDK project.

⚠️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.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ababra picture ababra  Β·  3Comments

peterdeme picture peterdeme  Β·  3Comments

EduardTheThird picture EduardTheThird  Β·  3Comments

abelmokadem picture abelmokadem  Β·  3Comments

pepastach picture pepastach  Β·  3Comments