Typescript examples fail standard code analysis regarding constructor side-effects

beanaroo commented 2 years ago

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

cdktf & Language Versions

cdktfL 0.9.4 Typescript: 4.6.2

Affected Resource(s)

All resources

Expected Behavior

Examples should not emit issues for conventional code quality rules.

Actual Behavior

ESLint emits no-new errors: https://eslint.org/docs/rules/no-new SonarQube emits Major Bug S1848: https://rules.sonarsource.com/typescript/RSPEC-1848

Steps to Reproduce

export class HelloTerra extends TerraformStack {
  constructor(scope: Construct, id: string) {
    super(scope, id);

    new AwsProvider(this, "aws", {   // Emits Issue
      region: "eu-central-1",
    });

    const region = new DataAwsRegion(this, "region");

    new aws.DynamoDB.DynamodbTable(this, "Hello", {   // Emits Issue
      name: `my-first-table-${region.name}`,
      hashKey: "temp",
      attribute: [{ name: "id", type: "S" }],
      billingMode: "PAY_PER_REQUEST",
    });
  }
}

Important Factoids

Searching the internet for information on these errors leads to advice that this is bad practice and should be avoided. This can be confusing for newcomers to the library.

Working in an organization that has shared/common linting rules makes it hard to justify adding exceptions.

It would be great if the documentation could address this, either through better examples or by clear explanation on why this approach is necessary.

References

jsteinich commented 2 years ago

You are declaring what resources you want in your infrastructure. In a way, each new statement is like creating the infrastructure resource. Sometimes other pieces reference that (and you assign to a variable), but other times nothing needs to reference them.

ansgarm commented 2 years ago

Hi @beanaroo 👋 I can see your reasoning here. While this pattern does not match conventional code quality rules, it is a core pattern of the underlying constructs library which powers all CDKs. Hence the AWS CDK and the cdk8s also have examples sharing this pattern.

While I couldn't find any material on this design decision (e.g. in the AWS CDK RFC repo), I suspect that this pattern has been chosen for simplicity and for the sentiment of new Something() which already suggests the "creation" of certain pieces of infrastructure.

I like your idea on adding docs for this which is certainly something we could cover and where we could advise to disable said linting rules in CDKTF projects.

hashicorp / terraform-cdk