refactor(core): improvements to Construct API #2767

eladb · 2019-06-06T07:26:40Z

Closes #1934

BREAKING CHANGE:

core: node.stack is now Stack.of(construct) (fixes Deprecate node.stack #2766)
core: node.resolve has been moved to stack.resolve.
core: node.stringifyJson has been moved to stack.stringifyJson.
core: node.validateTree is now ConstructNode.validate(node)
core: node.prepareTree is now ConstructNode.prepare(node)
core: node.getContext is now node.tryGetContext
core: node.recordReference is now node.addReference
core: node.apply is now node.applyAspect
core: node.ancestors() is now node.scopes
core: node.required has been removed.
core: node.typename has been removed.
core: node.addChild is now private
core: node.findReferences() is now node.references
core: node.findDependencies() is now node.dependencies
core: stack.dependencies() is now stack.dependencies
core: CfnElement.stackPath has been removed.
core: CloudFormationLang is now internal (use stack.toJsonString())

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Ensure documentation coverage and clean up of the various APIs. Fixes #1891 BREAKING CHANGE: * **core:** `StackProps.autoDeploy` has been removed and replaced by `StackProps.hide` (with negated logic). * **core:** `ISynthesizable.synthesize` now accepts an `ISynthesisSession` which contains the `CloudAssemblyBuilder` object. * **cx-api:** Multiple changes to the cloud assembly APIs to reduce surface area and clean up.

BREAKING CHANGE: - `StackProps.autoDeploy` renamed to `entrypoint` (still true by default). - CLI must be upgraded to 0.34.0 (cx protocol changes)

…nstruct

BREAKING CHANGE: * **core:** `node.validateTree` is now `ConstructNode.validate(node)` * **core:** `node.prepareTree` is now `ConstructNode.prepare(node)` * **core:** `node.getContext` is now `node.tryGetContext` * **core:** `node.recordReference` is now `node.addReference` * **core:** `node.apply` is now `node.applyAspect` * **core:** `node.ancestors()` is now `node.scopes` * **core:** `node.required` has been removed. * **core:** `node.typename` has been removed. * **core:** `node.addChild` is now private * **core:** `node.findReferences()` is now `node.references` * **core:** `node.findDependencieS()` is now `node.dependencies` * **core:** `stack.dependencies()` is now `stack.dependencies` * **core:** `construct.node.stack` is now `Stack.of(construct)` * **core:** `CfnElement.stackPath` has been removed.

BREAKING CHANGE: * **core:** `node.resolve` has been moved to `stack.resolve`. * **core:** `node.stringifyJson` has been moved to `stack.stringifyJson`.

rix0rrr · 2019-06-06T11:05:58Z

I think the last commit does things that aren't necessary:

node.resolve has been moved to stack.resolve.

Weren't we talking about removing resolve() altogether? Better to do that than to move it then.

node.stringifyJson has been moved to stack.stringifyJson.

Actually this method could go as well. It does nothing that CloudFormationLang.toJSON() doesn't do, so the usage sites could be replaced.

Yes, this couples it to CloudFormation (we lose target language abstraction), but in all instances where we're calling it, we're calling it from the context of a Construct Library resource which already knows it's going to be deployed using CloudFormation (same situation as where we would be using Stack.of()) so the coupling is justified.

eladb · 2019-06-06T11:10:40Z

Weren't we talking about removing resolve() altogether? Better to do that than to move it then.

I tried to look into moving resolve() into the synthesis session, and realized it's going to be too hard. It is needed in some "prepare" contexts as well and an infinite number of tests. I was concerned that it was under node, but under stack it feels better, especially given it uses CloudFormation functions for concatenations, etc. So I am comfortable with stack.resolve for now. If you feel strongly about it, I am willing to give this another try.

Actually this method could go as well. It does nothing that CloudFormationLang.toJSON() doesn't do, so the usage sites could be replaced.

I think stack.stringifyJson is more discoverable and easy to reason about than CloudFormationLang.toJSON() which we should make internal. And it is used quite a lot (every time there's a "document" resource property), and I believe customers will potentially use it with "leaky" custom resources as well.

packages/@aws-cdk/cdk/lib/stack.ts

RomainMuller · 2019-06-06T11:24:50Z

packages/@aws-cdk/cdk/lib/stack.ts

+  /**
+   * Convert an object, potentially containing tokens, to a JSON string
+   */
+  public stringifyJson(obj: any): string {


This does not stringify JSON, it stringifies stuff to CFN-y Json. I don't love that method name, and I kinda think stringify or toJsonString is better.

You are right. I like toJsonString.

@rix0rrr what do you think?

* stringifyJson => toJsonString * node.resolve => stack.resolve * CloudFormationLang is now toJsonString

mergify · 2019-09-23T15:05:33Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

Elad Ben-Israel added 16 commits June 4, 2019 02:13

Additional changes

7ab5fcb

BREAKING CHANGE: - `StackProps.autoDeploy` renamed to `entrypoint` (still true by default). - CLI must be upgraded to 0.34.0 (cx protocol changes)

CR fixes

7565b7e

Merge remote-tracking branch 'origin/master' into benisrae/revisit-app

4ec73ab

additional fixes

d9cb6f3

more fixes

96be493

Merge remote-tracking branch 'origin/master' into benisrae/revisit-app

76af774

remove "entrypoints" (autoDeploy)

27959e6

fix tests

7ae9720

fix integ test

731cf64

allow integ tests to specify which stack to deploy

c2cdd30

fix kms test and be less opinionated

0dc7bef

fix another multi-stack test

b758f23

add a @deprecated "app.run()"

530fc3d

Merge remote-tracking branch 'origin/master' into benisrae/revisit-co…

ebe1b11

…nstruct

eladb requested review from RomainMuller, skinny85 and a team as code owners June 6, 2019 07:26

fix cfn2ts

57baa9f

rix0rrr approved these changes Jun 6, 2019

View reviewed changes

RomainMuller approved these changes Jun 6, 2019

View reviewed changes

fix all stack.node occurrences

be6d475

eladb requested a review from SoManyHs as a code owner June 6, 2019 09:37

Elad Ben-Israel added 2 commits June 6, 2019 13:01

fix compilation issue

e4c0399

move "resolve" and "stringifyJson" to Stack

d282577

BREAKING CHANGE: * **core:** `node.resolve` has been moved to `stack.resolve`. * **core:** `node.stringifyJson` has been moved to `stack.stringifyJson`.

RomainMuller reviewed Jun 6, 2019

View reviewed changes

Misc

d8e0098

* stringifyJson => toJsonString * node.resolve => stack.resolve * CloudFormationLang is now toJsonString

Elad Ben-Israel added 2 commits June 6, 2019 17:00

and another one

a54f0b8

fix alb test

1a7c85c

eladb merged commit 3f7a0ad into master Jun 6, 2019

eladb deleted the benisrae/revisit-construct branch June 6, 2019 15:07

skinny85 mentioned this pull request Jun 10, 2019

fix(changelog): add missing entries from the 0.34.0 release to the Ch… #2814

Merged

4 tasks

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): improvements to Construct API #2767

refactor(core): improvements to Construct API #2767

eladb commented Jun 6, 2019 •

edited

Loading

rix0rrr commented Jun 6, 2019

eladb commented Jun 6, 2019

RomainMuller Jun 6, 2019

eladb Jun 6, 2019

eladb Jun 6, 2019

rix0rrr Jun 6, 2019

mergify bot commented Sep 23, 2019

refactor(core): improvements to Construct API #2767

refactor(core): improvements to Construct API #2767

Conversation

eladb commented Jun 6, 2019 • edited Loading

Pull Request Checklist

rix0rrr commented Jun 6, 2019

eladb commented Jun 6, 2019

RomainMuller Jun 6, 2019

Choose a reason for hiding this comment

eladb Jun 6, 2019

Choose a reason for hiding this comment

eladb Jun 6, 2019

Choose a reason for hiding this comment

rix0rrr Jun 6, 2019

Choose a reason for hiding this comment

mergify bot commented Sep 23, 2019

eladb commented Jun 6, 2019 •

edited

Loading