RFC: 0192 Removal of the "constructs" compatibility layer

[eladb]

• Tracking issue: Removal of the "constructs" compatibility layer (v2.0) #192
• Rendered version

---

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[RomainMuller]

I kinda feel this would be more consistent with the Aspects API and thus would incur less cognitive load to developers.

[iliapolo]

Yes, I like it too.

[eladb]

Yes I agree, Tag.add is the current API, as mentioned here we can consider changing to Tags.of(x).add(), but it's not a breaking change nor required for this migration at this point.

[RomainMuller]

I kinda liked Node.of(c) because it keeps the API of Construct/IConstruct super clean. However I don't really love how IConstruct is just a marker interface and it has no bearing in how Node.of(IConstruct) achieves getting the Node from there (which makes it unsafe).

[eladb]

Exactly, and that's why I am reintroducing IConstruct.node so it's not a marker interface anymore. Also, not having this.node as a way to access the node is going to break a lot of CDK code.

[iliapolo]

@eladb Notice: aws/aws-cdk#8645

Should we really have the addInfo, addWarning, ... methods in constructs? These are tied to the aws-cdk implementation, and don't have any meaning outside its context (for example for cdk8s).

Can we move this to @aws-cdk/core? Perhaps its worth extending constructs.Construct in @aws-cdk/core and providing this API, and have users still extend core.Construct rather than constructs.Construct?

Im thinking this intermediate core.Construct will eventually have additional API that is relevant to all aws-cdk constructs, but not to the generic Construct.

EDIT: I think we can disregard this as I see it was addressed in the RFC. I also agree that we should keep the composition model pure to constructs.Construct, and add any domain specific features another way.

[iliapolo]

Yes, I like it too.

[iliapolo]

I think an example for a Lazy (or whatever we most recommend for "late" bound calculations) implementation of something that would normally be in prepare would be nice.

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[RomainMuller]

So... let me summarize this: all classes that extend Construct will have a construct property that returns an instance of Node? I would naturally have expected construct to return an instance of Construct (basically, this), and the naming you're proposing strikes me as surprising and counter-intuitive.

[eladb]

The property name should be considered in the context of its usage rather than its type because the type is not part of the usage semantics.

Do you find this counterintuitive?

bucket.construct.addDependency(role);

As oppose to:

bucket.node.addDependency(role);

I actually think the former is much more intuitively and reads much better.

We actually have a naming guideline that says that names should not include the type name because it's redundant information in strongly typed environments.

Do you have an alternative suggestion?

Perhaps we can rename Node to something else?

[RomainMuller]

We actually have a naming guideline that says that names should not include the type name because it's redundant information in strongly typed environments.

The problem is the name is the base type name (of the receiver) as well as the coined term for sub-types of it. The reason I don't love it is precisely because of this guideline: it looks like redundant information (so why's it there?), but is not (so it's surprising in that sense).

---

As discussed... I don't feel extremely strong against this. It does feel counter-intuitive to me but I have no idea whether I'm representative of users or not in this respect.

For the record, I think this.construct.addDependency(role) reads super good, but for some reason the magic weakens when the receiver is not this.

Additionally, I cannot come up with a better suggestion for a name... so there's that, too.

[rix0rrr]

I know it's a little late, but I don't agree with this rename.

I would expect a field called construct to point to an object that IS a Construct. I like what they did in TerraCDK, constructNode seems like a good compromise.

[eladb]

The main reason we have this property is in order to group all the API of Construct under a single namespace so it won't "pollute" the APIs of subclasses, which are oftentimes generated classes in CDKs.

The conceptual mental model is that this is "the construct API". The fact that the type is Node is actually not in line with this mental model.

If I am looking at this from a usage point of view, I think construct makes more sense:

this.construct.id
this.construct.path
this.construct.addDependency(other)
this.construct.addMetadata

This is actually more natural then:

this.node.id
this.node.path
...

Additionally, we actually have a guideline against using the type name in symbol names because type information is an intrinsic part of the API (we prefer handler: lambda.Function over handlerFunction: lambda.Function).

I would rather rename the Node class (maybe ConstructApi) instead of renaming this property.

[rix0rrr]

For the record, after some discussion I think I would have preferred for this member to be called constructTree, as this is the member that consumers use to access the features of the "construct tree" (which is a CDK concept that people are, or should be, familiar with which is distinct from the concept of a "construct").

We get things like:

// All perfectly natural
bucket.constructTree.findChild()
bucket.constructTree.removeChild()
bucket.constructTree.defaultChild
bucket.constructTree.findAll()
bucket.constructTree.children
bucket.constructTree.addWarning()

// Better than what we have now, this name emphasizes the tree traversal involved in these operations
bucket.constructTree.setContext()
bucket.constructTree.applyAspect()
bucket.constructTree.addDependency() 
 
// These now have not such great names, because they apply to the "current node" and don't really have
// strong tree-ish connotations. Don't think any of them are *horrible* but they are certainly not fantastic.
bucket.constructTree.id
bucket.constructTree.path
bucket.constructTree.uniqueId

[eladb]

I don't think CDK users are required to understand the tree concept. In fact, we have traditionally tried to talk about "scopes" (I.e "the construct is created within the scope of another construct"). From a CS perspective this is indeed a tree, but I don't see how this adds much value to users.

As mentioned above, the primary reason for this property is to group all the construct APIs under an api namespace so they don't pollute or conflict the public api of subclasses, which is the primary programming pattern we use in CDKs.

[rix0rrr]

There's a difference between not needing to know that there is a tree structure involved (until you do), and pretending that the tree structure doesn't exist by studiously avoiding calling it a tree.

You might be under the impression that my argument is one about purity of CS concepts. It is not.

It is about acknowledging the fact that this structure exists, that it underpins the way the CDK works in a way which you can oftentimes ignore, but sometimes is quite relevant and requires people to understand it (see Stages, Stacks, Aspects, LogicalIDs, default children and escape hatches, see tree.json and the VSCode plugin, ...) and that there is a term available which accurately describes the semantics, expectations, and the sort of API that people can expect from this structure that they are already intimately familiar with from other programming tasks.

I think being vague and coy and pretending there is no tree when we all full well know that there is, or using the word "construct" to refer to two distinct concepts, is not going to help users form a mental model.

I'm also not saying they won't be able to. Given enough exposure and enough examples, over time people are going to eventually be trained to write the right code. In that sense, ultimately it doesn't even matter what it's called, be it node, construct, or abc, people will learn to cope.

---

I think I've laid out my reasoning for why I think it's advantageous to call the beast by its name. It's completely fine if you disagree.

[eladb]

Cool, thanks for the effort in explaining.

The plan is to introduce construct and deprecate node over the 1.x branch in short order. This will give us some "bake time" of a few months until we completely remove node in 2.x. This will allow us to get feedback from users in case the mental model is confusing or insufficient.

[skorfmann]

Just stumbled upon this - thanks for making the effort of changing the name 👍

Pull request has been modified.