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

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

Merged
merged 9 commits into from
Oct 30, 2024

Conversation

ren-yamanashi
Copy link
Contributor

@ren-yamanashi ren-yamanashi commented Oct 29, 2024

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

  • noImplicitAny
  • strictNullChecks
  • noImplicitThis
  • alwaysStrict

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

  • target
    • I think 'ES2023' is more appropriate for the target option at this point.
  • lib

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

@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Oct 29, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team October 29, 2024 13:33
@ren-yamanashi ren-yamanashi changed the title chore(init-templates): update typescript/tsconfig.json chore(init-templates): update tsconfig.json Oct 29, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 29, 2024
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

strictPropertyInitialization can also be removed.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 29, 2024
@mergify mergify bot dismissed mrgrain’s stale review October 30, 2024 00:19

Pull request has been modified.

@ren-yamanashi
Copy link
Contributor Author

@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. 6a66600

@mrgrain
Copy link
Contributor

mrgrain commented Oct 30, 2024

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. 6a66600

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 30, 2024
@mrgrain mrgrain changed the title chore(init-templates): update tsconfig.json feat(cdk): update tsconfig to more modern settings in typescript init templates Oct 30, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 30, 2024
@ren-yamanashi ren-yamanashi force-pushed the update-typescript-init-template branch from 6a66600 to 8c3d56b Compare October 30, 2024 10:07
@ren-yamanashi
Copy link
Contributor Author

ren-yamanashi commented Oct 30, 2024

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

@mrgrain mrgrain added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Oct 30, 2024
@mrgrain
Copy link
Contributor

mrgrain commented Oct 30, 2024

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 aws-cdk-automation dismissed their stale review October 30, 2024 10:21

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

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

Copy link
Contributor

mergify bot commented Oct 30, 2024

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: f2a66d4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 8e4c247 into aws:main Oct 30, 2024
11 of 12 checks passed
Copy link
Contributor

mergify bot commented Oct 30, 2024

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).

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants