-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Incorrect template for cloudwatch dashboard #6746
Comments
This looks very weird indeed. Can you provide a minimal reproducing sample? |
@rix0rrr I have already added the code which can lead to the problem. You can put it into sample-app and then run "cdk synth". |
Yes, you are right, sorry. I thought the sample was incomplete because Investigating. |
The issue seems to be that we are trying to serialize a Token (
The rest of the logging is not very useful but it comes down to the serializer serializing the individual fields of the token (because it's treating the token as a data type). |
BTW confirmed that the same code works fine in TypeScript. |
A "fix" would be to serialize by reference if any of the fields are functions...? Or if the object has a prototype chain? |
Somehow, it appears that none of the methods of the A "quick fix" (CDK-side) is to introduce some jsii-exported base class and have Longer term - I like your idea of checking for a prototype chain and if there is one. We'll have to move carefully to make sure this does not cause any regression... So it'll probably take a little longer... |
Prototype chain is not good enough though, it would miss objects defined like this: return {
value: 'hello',
alsoFunction() { /* do something */ },
};
return {
value: 'hello',
alsoFunction: () => { /* do something */ },
}; I think the only thing to do is add a check just before https://github.com/aws/jsii/blob/a2da0bfce356c2332232808d4d6eaccaf7c6d4ad/packages/%40jsii/kernel/lib/serialization.ts#L486 where we iterate the object for functions, and not just its own properties but including its prototype chain. |
When a "private" type (aka a class that is not visible in the `.jsii` assembly) is passed from JS to Host through an `any`-typed entity, the value was serialized by-value, passing only the instance's properties, and omitting any methods. This changes the behavior of the `SerializationClass.Any` serializer, so that when an anonymous object is encountered, if it has a dynamic getter, setter or any method (including a constructor), it is passed by-reference instead of by-value. Fixes aws/aws-cdk#6746
I have a candidate fix in aws/jsii#1347. I need to make sure it does not break some of the existing APIs that return It works by looking up the prototype chain for methods or dynamic property accessors (we cannot guarantee those are stable, so they get by-ref treatment, too -- it's safer). |
When a "private" type (aka a class that is not visible in the `.jsii` assembly) is passed from JS to Host through an `any`-typed entity, the value was serialized by-value, passing only the instance's properties, and omitting any methods. This changes the behavior of the `SerializationClass.Any` serializer, so that when an anonymous object is encountered, if it has a dynamic getter, setter or any method (including a constructor), it is passed by-reference instead of by-value. Fixes aws/aws-cdk#6746
@RomainMuller Do you have any information which version of CDK will contain the fix for this problem? And when? I tried on 1.31.0 and the problem is reproduced. |
@DamirAinullin the fix landed in 1.2.0 which we are working on upgrading CDK to use. This should go out in the next release. |
@MrArnoldPalmer Try to use CDK version 1.34.1. And get app crash during cdk deploy with the following exception:
Can you look at it please? The reproduction code is the same as at the begining. |
I try to use CDK to create AWS monitoring dasboard and get problem with incorrect template.
Reproduction Steps
Error Log
The problem starts with the metrics attribute.
When I run "cdk synth" I see incorrect template with such part for metrics:
Environment
Other
I suppose it somehow related to this problem:
#5735
This is 🐛 Bug Report
The text was updated successfully, but these errors were encountered: