search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.68k stars 3.93k forks source link

feat(cdk): update tsconfig to more modern settings in typescript init templates #31927

Closed ren-yamanashi closed 2 weeks ago

ren-yamanashi commented 2 weeks ago

Issue # (if applicable)

n/A

Reason for this change

I think contents of tsconfig.json are a little outdated and seemed to need update.

Description of changes

The following two changes I made:

  1. Removed unnecessary options
  2. Updated target and lib options at compilerOptions

1. Removed unnecessary options

Unnecessary options included in the compilerOptions of tsconfig.json.
The unnecessary options are as follows

If strict: true is specified in compilerOptions, these options will also be true, so there is no need to specify them.

see: https://www.typescriptlang.org/tsconfig/#strict

2. Updated target and lib options at compilerOptions

Description of how you validated changes

Checklist

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

ren-yamanashi commented 2 weeks ago

@mrgrain

Nice! Going by this https://www.npmjs.com/package/@tsconfig/node-lts we should set the target to ES2022 and lib to include ES2023.

Thank you review! I fixed in the following commit.

strictPropertyInitialization can also be removed.

strictPropertyInitialization is currently set to false, but is it okay to remove it?
Note: If strict: true is set, strictPropertyInitialization will implicitly become true.

If it is better not to remove, I will reset this commit. https://github.com/aws/aws-cdk/pull/31927/commits/6a66600011ba906133e3b5fc1ba031eb6ef2a28d

mrgrain commented 2 weeks ago

strictPropertyInitialization can also be removed.

strictPropertyInitialization is currently set to false, but is it okay to remove it?
Note: If strict: true is set, strictPropertyInitialization will implicitly become true.

If it is better not to remove, I will reset this commit. https://github.com/aws/aws-cdk/pull/31927/commits/6a66600011ba906133e3b5fc1ba031eb6ef2a28d

Ha nice catch. Let's keep it set to false then for now.

ren-yamanashi commented 2 weeks ago

Ha nice catch. Let's keep it set to false then for now.

@mrgrain

I have set strictPropertyInitialization back to false, so please check.

Also, there is no README and test, and CI seems to be failing.
Should I address these issues?

The PR that made a similar change to this PR did not seem to have updated the README or test...
https://github.com/aws/aws-cdk/pull/27422

mrgrain commented 2 weeks ago

Ha nice catch. Let's keep it set to false then for now.

@mrgrain

I have set strictPropertyInitialization back to false, so please check.

Also, there is no README and test, and CI seems to be failing. Should I address these issues?

The PR that made a similar change to this PR did not seem to have updated the README or test... #27422

Great! I can take care of the PR "requirements" ;)

Disabled readme and test reqs. This is a change to code that has existing tests. Will run through the cli test pipeline.

aws-cdk-automation commented 2 weeks ago

:arrow_right: PR build request submitted to test-main-pipeline :arrow_left:

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

mergify[bot] commented 2 weeks ago

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation commented 2 weeks ago

AWS CodeBuild CI Report

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify[bot] commented 2 weeks ago

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions[bot] commented 2 weeks ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.