Expose nodeFactory in construct-compat

[kirtfitzpatrick]

Could we expose the nodeFactory option for constructs.Construct up one level in the old aws-cdk Construct?

I'm sure it's only there for backwards compatibility but I think I'm always going to want the option of supplying a nodeFactory to the superclass when I subclass Construct and Stack. The node is where all the naming and path manipulation happen and I can't manipulate any of that if I can't subclass ConstructNode. And as far as I can tell I can't subclass that in a meaningful way without a factory. (I'm guessing that's why you went with a nodeFactory option as well.)

I've looked into addPropertyOverride and overrideLogicalId but neither seem to do what I need.

constructs.construct.ts

  constructor(scope: Construct, id: string, options: ConstructOptions = { }) {
    // attach the construct to the construct tree by creating a node
    const nodeFactory = options.nodeFactory ?? { createNode: (host, scope, id) => new Node(host, scope, id) };
    Object.defineProperty(this, CONSTRUCT_NODE_PROPERTY_SYMBOL, {
      value: nodeFactory.createNode(this, scope, id),

aws-cdk.construct-compat.ts

  constructor(scope: Construct, id: string) {
    super(scope, id, {
      nodeFactory: {
        createNode: (h: constructs.Construct, s: constructs.IConstruct, i: string) =>
          new ConstructNode(h as Construct, s as IConstruct, i)._actualNode,
      },
    });

I haven't tried it yet but it seems to me that it could be implemented in a backwards compatible way with an optional argument.

• 👋 I may be able to implement this feature request
• ⚠️ This feature might incur a breaking change

---

This is a 🚀 Feature Request

[kirtfitzpatrick]

Hey @eladb, I've already done some work related to this locally. I could probably implement it if you approve?

[eladb]

Hi, sorry for the late response.

I’d like to understand your use cases more. Can you describe exactly what you need that requires subclassing ConstructNode?

I am not sure exposing nodeFactory is the best way and it introduces a major api surface that I wouldn’t want to open up without a very solid case, because it makes it much harder for us to iterate on the internals.

[kirtfitzpatrick]

Which surface are you most concerned with exposing? Node or NodeFactory?

There have been a number of issues opened against cdk regarding naming. Couldn't all of those issues be resolved in an elegant and powerful way by allowing constructs to bring their own node?

Most recently I've been experimenting with Constructs as a way to organize and reuse stacks. Everything works perfectly except for stack names. The implementation of Node assumes that Stack is always at the base+1 location of the tree and uses that assumption to determine how to name it.

To me, this doesn't seem like the right place to make that decision, no? Shouldn't the node know its construct is a special case instead of guessing? Taking it a step further, shouldn't a new Construct sub-class also be able to tell its node it's a special case?

For example:
readme
code

And in the generated stack names below:

$ cdk list
FnpRegistryB3019273
FnpDevNetwork67514EC7
FnpDevPublicEcs45E75100
FnpDevHelloDocker434A33B3
FnpDevTweetIngestC12A26E5

To be honest, I want far more control over naming than what I mention here. And this isn't another issue complaining about the unique hash. I get the hash. I believe in the hash. I use the hash. For the most part I've given up on what I want and tried to do things "the cdk way". Even so I find myself struggling to create something orderly at times.

What are your thoughts @eladb ?

[eladb]

There have been a number of issues opened against cdk regarding naming. Couldn't all of those issues be resolved in an elegant and powerful way by allowing constructs to bring their own node?

I don't think all of those issues can be solved by subclassing ConstructNode. For example, the stack name logic is implemented in Stack, not in ConstructNode.

Specifically for the use case you describe, why not just explicitly pass stackName when you define your stacks within the construct:

new Stack(this, 'foobar', {
  stackName: 'foo'
});

[eladb]

Closing this. Please ping if there are other use cases that you are unable to address.

[kirtfitzpatrick]

Hey @eladb, sorry for the delay. I had to switch to another project.

Setting stack_name is not ideal for two reasons:

1. It clobbers the scope created by the parent constructs. Solving this necessitates duplicating the node naming logic in my own code. I've looked at that code, it has a lot of edge cases. You can see my quick and dirty attempt at this here.
2. CDK doesn't respect stack_name. Setting the stack name changes the stack resource generated in AWS, but all the cdk cli commands and CF templates use the name generated by node. This is confusing, slow to work with on the command line and I'm concerned would be a likely source of bugs if the code is used as part of a more autonomous system. There's no way to know where I should use the stack_name to refer to the stack and where to use the node generated name.

A single source of truth would be better.

Regarding other use cases, one example is I'd prefer to lowercase names and separate scopes with a dash. I don't think that's possible but please correct me if I'm wrong.

[kirtfitzpatrick]

Given the following example app.py:

from aws_cdk import core

app = core.App()
foo_construct = core.Construct(app, "Foo")
bar_stack = core.Stack(foo_construct, "Bar", stack_name="Bar")
app.synth()

cdk command produces these outputs:

$ cdk list
FooBarCFC0D4D9
$ ls -1 cdk.out/*template*
cdk.out/FooBarCFC0D4D9.template.json

[eladb]

I still believe the actual deployed stack name in this case will be "Bar". That's at least what's supposed to happen. The name of artifact is intentionally based on the construct path to enforce uniqueness and allow you to have multiple stacks with the same name deployed to different AWS environments (see details #4895)

[kirtfitzpatrick]

Yes. I mentioned that as "It clobbers the scope created by the parent constructs." and "Setting the stack name changes the stack resource generated in AWS" I don't have an issue with what "stack_name" does, but a solution to my problem it is not.

This issue is about granting access to the node, working with the name generation code, single source of truth and using the same name for the same thing everywhere. Could you speak to my issue?

[eladb]

Sorry, I missed your other comment.

It clobbers the scope created by the parent constructs. Solving this necessitates duplicating the node naming logic in my own code. I've looked at that code, it has a lot of edge cases. You can see my quick and dirty attempt at this here.

Not sure what you mean by "clobbers the scope created by parent constructs". Can you elaborate?

CDK doesn't respect stack_name. Setting the stack name changes the stack resource generated in AWS, but all the cdk cli commands and CF templates use the name generated by node. This is confusing, slow to work with on the command line and I'm concerned would be a likely source of bugs if the code is used as part of a more autonomous system. There's no way to know where I should use the stack_name to refer to the stack and where to use the node generated name.

We have an issue to be able to use stack names in the CLI: #8390

Regarding other use cases, one example is I'd prefer to lowercase names and separate scopes with a dash. I don't think that's possible but please correct me if I'm wrong.

Generally, CFN does not support dashes in logical names, but you can determine how logical names are assigned to resources in a stack by overriding allocateLogicalId at the stack level. You can create your own subclass of Stack to encode this policy once and use it throughout your application.

[kirtfitzpatrick]

Interesting. allocate logical id doesn't appear to be documented in the Python docs for Stack. Is this intended? (Alternatively, I might be blind and simply not seeing it.)

Either way, I noticed the docs referring to it were apparently incorrect up until 2 weeks ago. This might be just what I need. I'll play around with it and report back. Thanks for pointing it out!

Not sure what you mean by "clobbers the scope created by parent constructs". Can you elaborate?

By "clobbers" I mean stack_name ignores the scope and the unique hash systems. I believe this is the intended behavior for setting the stack_name property but it doesn't serve my use case.

[eladb]

By "clobbers" I mean stack_name ignores the scope and the unique hash systems. I believe this is the intended behavior for setting the stack_name property but it doesn't serve my use case.

I am still not sure I understand your use case. Can you please describe it again? If stack_name is not specified, it will default to the artifact ID which does take into account the scope & unique hash systems.