feat(cdk): construct.uniqueId and relax construct id constraints #556

eladb · 2018-08-14T06:26:15Z

Adds a property uniqueId to Construct which returns an alphanumeric 255-length-limited tree-unique identity for a construct.

Relax constraints for construct id (previously known as "name") to only restrict the usage of the path separator. Otherwise, all characters are allowed. This will allow using the construct id for a wider range of purposes, but since logical IDs (and uniqueId now) are alpha-safe, it's okay.

Deprecate construct.name in favor of construct.id.

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

Adds a property `uniqueId` to Construct which returns an alphanumeric 255-length-limited tree-unique identity for a construct. Relax constraints for construct id (previously known as "name") to only restrict the usage of the path separator. Otherwise, all characters are allowed. This will allow using the construct id for a wider range of purposes, but since logical IDs (and uniqueId now) are alpha-safe, it's okay. Deprecate `construct.name` in favor of `construct.id`.

RomainMuller · 2018-08-14T07:20:41Z

packages/@aws-cdk/cdk/lib/cloudformation/logical-id.ts

@@ -1,11 +1,7 @@
+import { makeUniqueId } from '../util/uniqueid';
 import { StackElement } from "./stack";


Update to use single quotes.

RomainMuller · 2018-08-14T07:21:48Z

packages/@aws-cdk/cdk/lib/core/construct.ts

    /**
     * Returns the parent of this node or undefined if this is a root node.
     */
    public readonly parent?: Construct;

    /**
-     * The name of this construct
+     * @deprecated use `id`


Yeahhhhhh that's a poor deprecation message. You want to explain why id is better.

Also a good idea to mention when it was deprecated, so we can assess when it is safe to drop.

Finally, why are you so careful in not breaking usage of .name? I feel like this is probably not common place... Would have dropped.

okay, removed!

RomainMuller · 2018-08-14T07:22:07Z

packages/@aws-cdk/cdk/lib/core/construct.ts

     */
    public readonly name: string;

+    /**
+     * The subtree-local id of the construct.
+     * This id is unique within the subtree. To obtain a tree-unique id, use `uniqueId`.


Define subtree... The construct may be accessible from several parts of the tree, but I suppose the subtree in question is the one it's attached to, not the one you're accessing from...

Changed description to:

/** * The local id of the construct. * This id is unique amongst its siblings. * To obtain a tree-global unique id for this construct, use `uniqueId`. */ public readonly id: string;

RomainMuller · 2018-08-14T07:26:05Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+
+    /**
+     * The full path of this construct in the tree.
+     * Components are separated by '/'.


In other words:

So we had this jolly good array of strings that represent a path in the tree. We could have exposed it as a ReadonlyArray<string>, but we though - oh the hell, we can .join('/') this so in case you're interested in the array, you'll always have to .split('/') it. There there, we fixed it.

Yes, I know not every language supports ReadonlyArray<T>, but they could code-generate a copy-on-read and be good with it.

The reason this is a string is because I think that in most cases, when people need the path, they want the string version of it and not the components. Optimize for the common case.

RomainMuller · 2018-08-14T07:26:47Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+    public readonly path: string;
+
+    /**
+     * A tree-unique alpha-numeric identifier for this construct.


Hmm'kay. I feel tree is maybe over-used and maybe too generic.

Tree represents the entire construct tree. Added "global"

RomainMuller · 2018-08-14T07:38:02Z

packages/@aws-cdk/cdk/lib/util/uniqueid.ts

+    const hash = pathHash(components);
+    const human = removeDupes(components)
+        .map(removeNonAlpha)
+        .filter(x => x !== HIDDEN_FROM_HUMAN_ID)


Do you not want to do this one before removeNonAlpha? I feel that Re-source shouldn't be considered as === 'Resource'.

RomainMuller · 2018-08-14T07:39:17Z

packages/@aws-cdk/cdk/lib/util/uniqueid.ts

+/**
+ * Removes all non-alphanumeric characters in a string.
+ */
+function removeNonAlpha(s: string) {


Rename to removeNonAlphanumeric. The name is, otherwise, a lie.

RomainMuller · 2018-08-14T07:39:50Z

packages/@aws-cdk/cdk/lib/util/uniqueid.ts

+ * Removes all non-alphanumeric characters in a string.
+ */
+function removeNonAlpha(s: string) {
+    return s.replace(/[^A-Za-z0-9]/g, '');


Can make this return s.replace(/[^a-z0-9]/ig, '');

I prefer this

RomainMuller · 2018-08-14T07:42:18Z

packages/@aws-cdk/cdk/lib/util/uniqueid.ts

+/**
+ * Remove duplicate "terms" from the path list
+ *
+ * If a component name is completely the same as the suffix of


Very convoluted phrasing:

completely the same as the suffix of the previous comment name

How about

If the previous path component name ends with this component name, skip the current component.

RomainMuller · 2018-08-14T07:43:36Z

packages/@aws-cdk/cdk/test/core/test.jsx.tsx

-            <MyConstruct name='child1' prop1='hi' prop2={21} >
-                <MyConstruct name='child11' prop1='there' />
-                <MyConstruct name='child12' prop1='boo' />
+            <MyConstruct id='child1' prop1='hi' prop2={21} >


That sure is one place where id looks so much better! 👌🏻

* Use node.js "crypto" module instead of md5 * Remove `construct.name` (and fix all dependencies) * Remove non-alphanumeric after "Resource" is omitted BREAKING CHANGE: `construct.name` => `construct.id`

@rix0rrr

### Features * __@aws-cdk/cdk__: Tokens can now be transparently embedded into strings and encoded into JSON without losing their semantics. This makes it possible to treat late-bound (deploy-time) values as if they were regular strings ([@rix0rrr] in [#518](#518)). * __@aws-cdk/aws-s3__: add support for bucket notifications to Lambda, SNS, and SQS targets ([@eladb] in [#201](#201), [#560](#560), [#561](#561), [#564](#564)) * __@aws-cdk/cdk__: non-alphanumeric characters can now be used as construct identifiers ([@eladb] in [#556](#556)) * __@aws-cdk/aws-iam__: add support for `maxSessionDuration` for Roles ([@eladb] in [#545](#545)). ### Changes * __@aws-cdk/aws-lambda__ (_**BREAKING**_): most classes renamed to be shorter and more in line with official service naming (`Lambda` renamed to `Function` or ommitted) ([@eladb] in [#550](#550)) * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): move all CodePipeline actions from `@aws-cdk/aws-xxx-codepipeline` packages into the regular `@aws-cdk/aws-xxx` service packages ([@skinny85] in [#459](#459)). * __@aws-cdk/aws-custom-resources__ (_**BREAKING**_): package was removed, and the Custom Resource construct added to the __@aws-cdk/aws-cloudformation__ package ([@rix0rrr] in [#513](#513)) ### Fixes * __@aws-cdk/aws-lambda__: Lambdas that are triggered by CloudWatch Events now show up in the console, and can only be triggered the indicated Event Rule. _**BREAKING**_ for middleware writers (as this introduces an API change), but transparent to regular consumers ([@eladb] in [#558](#558)) * __@aws-cdk/aws-codecommit__: fix a bug where `pollForSourceChanges` could not be set to `false` ([@maciejwalkowiak] in [#534](#534)) * __aws-cdk__: don't fail if the `~/.aws/credentials` file is missing ([@RomainMuller] in [#541](#541)) * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support TemplateConfiguration ([@mindstorms6] in [#571](#571)). * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support ParameterOverrides ([@mindstorms6] in [#574](#574)). ### Known Issues * `cdk init` will try to init a `git` repository and fail if no global `user.name` and `user.email` have been configured.

eladb requested a review from rix0rrr August 14, 2018 06:26

rix0rrr approved these changes Aug 14, 2018

View reviewed changes

eladb merged commit 0efe25b into master Aug 14, 2018

eladb deleted the benisrae/construct-unique-id branch August 14, 2018 07:36

RomainMuller reviewed Aug 14, 2018

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdk): construct.uniqueId and relax construct id constraints #556

feat(cdk): construct.uniqueId and relax construct id constraints #556

eladb commented Aug 14, 2018

RomainMuller Aug 14, 2018

RomainMuller Aug 14, 2018

RomainMuller Aug 14, 2018

RomainMuller Aug 14, 2018

eladb Aug 14, 2018

RomainMuller Aug 14, 2018

eladb Aug 14, 2018

RomainMuller Aug 14, 2018

RomainMuller Aug 14, 2018

eladb Aug 14, 2018

RomainMuller Aug 14, 2018

eladb Aug 14, 2018

RomainMuller Aug 14, 2018

eladb Aug 14, 2018

RomainMuller Aug 14, 2018

RomainMuller Aug 14, 2018

eladb Aug 14, 2018

RomainMuller Aug 14, 2018

RomainMuller Aug 14, 2018

eladb Aug 14, 2018

		@@ -1,11 +1,7 @@
		import { makeUniqueId } from '../util/uniqueid';
		import { StackElement } from "./stack";

feat(cdk): construct.uniqueId and relax construct id constraints #556

feat(cdk): construct.uniqueId and relax construct id constraints #556

Conversation

eladb commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment