Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider if we should stringify non-string tokens #744

Closed
eladb opened this issue Sep 19, 2018 · 3 comments
Closed

Consider if we should stringify non-string tokens #744

eladb opened this issue Sep 19, 2018 · 3 comments
Labels
@aws-cdk/core Related to core CDK functionality pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version.

Comments

@eladb
Copy link
Contributor

eladb commented Sep 19, 2018

Following up on #695 and #712, which converted string attributes from strong-typed tokens to stringified tokens, we should consider what to do with other attribute types.

Namely, there are a few attributes in the CloudFormation spec that return string arrays and there one that returns JSON.

Also, we might want to consider what to do for tokens that represent numbers. There are non at the moment in CloudFormation, but an example would be port number.

eladb pushed a commit that referenced this issue Sep 19, 2018
…g types (#712)

This change removes all strong-types generated to represent 
CloudFormation resource attributes that return a string  (such 
as `QueueArn`, `DeploymentGroupId`) and replaces them 
with `string` that contains a stringified token (via #518).

This allows working with these attributes as if they were regular
strings, and dramatically simplifies the type-system and unneeded
wrapping when assigning values to such attributes.

The ".ref" property of each resource has been replaced with
a property named according to the actual semantic meaning
of the intrinsic reference (such as `queueArn`), and 
also returns a `string`.

Users can test if a string contains stringified tokens using the function 
`unresolved(o)`, which can be applied to either strings or objects 
(previously was `isToken(o)`).

Fixes #695 (opened #744 to follow up on non-string attributes)
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 20, 2018

I would say, same as for string values: we identify a range of special values in the domain that we use to represent tokens.

For numbers, we have two options:

  • Hide information in the unused bits of a NaN. That works in-memory (and is in fact sort-of what this unused space was intended for), but we'll have to take care at the JSII border when serializing them, to make it explicit that it's a NaN-w-extra information and JSII front-end implementors will have to carry these bits around as well. Might not be super portable.

  • Shrink the range of the double. We can reserve some bits at the upper or lower ranges of the range to encode token numbers. This is probably more portable than the previous one, but again we have to pay attention in jsii land that (a) all languages use doubles, not floats and (b) the stringification of the double contains enough detail that all bits are preserved and properly deserialized.

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 20, 2018

For lists, we'll have a special singleton list (["${THISISATOKENLIST}"]). However, the operations on lists are no longer trivial. The built-in operations MUST NOT be used anymore, because they cannot do the right thing in all cases.

Instead, we'll have to have functions which people use to operate on arrays, which either do the actual operation on a literal string list, or return a CloudFormation intrinsic in case of a tokenized list.

  • array.push(), array.concat(): prohibited, CloudFormation has no equivalent.
  • array.length: prohibited, CloudFormation has no equivalent.
  • array[5]: translates to Fn.select().
  • array.join(): maps to Fn.join()

We could potentially use ES6 Proxies to make this work transparently in JavaScript land (user cannot tell the difference and don't have to resort to special functions), but we'd have to stop serializing arrays as "just data" in that case and treat them like interfaces into jsii-land (think List<string> instead of string[]).

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 20, 2018

That reminds me, string.split() must also not be used anymore today, since it cannot work in the face of tokenized strings. It should produce a Fn.split() when encountering a Tokenized string (that you're NOT allowed to take the length of 😝 )

eladb pushed a commit that referenced this issue Sep 20, 2018
__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
eladb pushed a commit that referenced this issue Sep 20, 2018
* v0.9.2

__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
@fulghum fulghum added pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. @aws-cdk/core Related to core CDK functionality design labels Sep 20, 2018
rix0rrr added a commit that referenced this issue Nov 12, 2018
Add the ability for Tokens to be encoded into lists using the
`token.toList()` method.

This is useful for Tokens that intrinsically represent lists of strings,
so we can pass them around in the type system as `string[]` (and
interchange them with literal `string[]` values).

The encoding is reversible just like string encodings are reversible.
Contrary to strings encodings, concatenation operations are not
allowed (they cannot be applied after decoding since CloudFormation
does not have any list concatenation operators).

This change does not change any CloudFormation resources yet to
take advantage of the new encoding.

Implements the most important remaining part of #744.
rix0rrr added a commit that referenced this issue Nov 12, 2018
Add the ability for Tokens to be encoded into lists using the
`token.toList()` method.

This is useful for Tokens that intrinsically represent lists of strings,
so we can pass them around in the type system as `string[]` (and
interchange them with literal `string[]` values).

The encoding is reversible just like string encodings are reversible.
Contrary to strings encodings, concatenation operations are not
allowed (they cannot be applied after decoding since CloudFormation
does not have any list concatenation operators).

This change does not change any CloudFormation resources yet to
take advantage of the new encoding.

Implements the most important remaining part of #744.
@eladb eladb closed this as completed Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version.
Projects
None yet
Development

No branches or pull requests

4 participants