refactor(core): refactor CloudFormationLang.toJSON()
#11224
Conversation
|
|
rix0rrr
force-pushed
the
huijbers/refactor-tojson
branch
from
ef37632
to
53bf0b6
Compare
Our previous implementation of `toJSON()` was quite hacky. It replaced values inside the structure with objects that had a custom `toJSON()` serializer, and then called `JSON.stringify()` on the result. The resulting JSON would have special markers in it where the Token values would be string-substituted back in. It's actually easier and gives us more control to just implement JSONification ourselves in a Token-aware recursive function. This change has been split off from a larger, upcoming PR in order to make the individual reviews smaller.
rix0rrr
force-pushed
the
huijbers/refactor-tojson
branch
from
53bf0b6
to
cc4ad0f
Compare
| [key, value] = value; | ||
| } | ||
|
|
||
| recurse(key); |
There was a problem hiding this comment.
Is there a case where key is not a string? Should we be defensive against it?
There was a problem hiding this comment.
Object.entries() will always and only return strings.
| // Treat as an intrinsic if this LOOKS like a CFN intrinsic (`{ Ref: ... }`) | ||
| // AND it's the result of a token resolution. Otherwise, we just treat this | ||
| // value as a regular old JSON object (that happens to look a lot like an intrinsic). | ||
| if (isIntrinsic(obj) && resolvedTypeHint(obj)) { |
There was a problem hiding this comment.
doesn't isIntrinsic() impliy resolvedTypeHint() always returns true? Maybe just assert?
There was a problem hiding this comment.
We can have type hints on things that aren't intrinsics (for example, as returned by a lazy).
We can also have structures that LOOK like intrinsics but shouldn't be treated as such (for example, the literal structure { Ref: X })
There was a problem hiding this comment.
We can have type hints on things that aren't intrinsics (for example, as returned by a lazy).
This conditional won't cover non-intrinsics (it's an &&).
We can also have structures that LOOK like intrinsics but shouldn't be treated as such (for example, the literal structure { Ref: X })
Shouldn't isIntrinsic() cover this?
| } | ||
|
|
||
| /** | ||
| * Quote string literals inside an intrinsic |
There was a problem hiding this comment.
I still feel I don't understand why this is needed. Maybe some examples? Why isn't it possible to just use JSON.stringify() of the intrinsic ?
There was a problem hiding this comment.
I will add more realistic tests.
There was a problem hiding this comment.
There was a problem hiding this comment.
Your intuition was correct though. I missed some edge cases. After thinking about it some more, just a straight up recursion over all strings should also do it. Added more tests to confirm.
| @@ -232,22 +292,47 @@ nodeunitShim({ | |||
| }, | |||
| }, | |||
| }); | |||
| }); | |||
|
|
|||
| test('Intrinsics can occur in key position', () => { | |||
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Our previous implementation of
toJSON()was quite hacky.It replaced values inside the structure with objects that had a custom
toJSON()serializer, and then calledJSON.stringify()on the result.The resulting JSON would have special markers in it where the Token
values would be string-substituted back in.
It's actually easier and gives us more control to just implement
JSONification ourselves in a Token-aware recursive function.
This change has been split off from a larger, upcoming PR in order
to make the individual reviews smaller.
Incidentally also fixes #13465, as the type of encoded tokens is assumed to match
the type of the encoded value (e.g., a
string[]-encoded token is assumed toproduce a list at deploy-time and so will not be quoted).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license