refactor: make logic more concise with optional chaining and nullish coalescing operators #12277
Conversation
…lace of existing logic
|
|
|
@flemjame-at-amazon this is a very impressive PR, thanks for the contribution! Can I ask: did you do the changes manually, or did you employee some automated way to perform these refactorings? |
I did it manually because parsing code with code isn't my forte, but it was pretty simple to find them all and replace. The main grep was: grep -r "undefined ? " --include="*.ts" packages/\@aws-cdk/aws-ec2 | grep -v generated | grep -v '{' | lessFor each module, then do a build on that module to check that there was no subtle logic differences. |
|
@skinny85 can this be merged? I ask because it covers so many files that it's likely that someone will commit a change which creates a merge conflict |
|
@nija-at can you weigh in here? Does this increase the chance of merge conflicts with |
|
Thanks for checking in. This shouldn't cause merge conflicts. |
There was a problem hiding this comment.
Thanks so much for your hard work on this @flemjame-at-amazon !
|
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). |
…coalescing operators (aws#12277) This is an unnecessary refactor that shrinks logic across CDK, mostly by replacing phrases like: ```ts const var = props.property !== undefined ? props.property : 'defaultValue'; const var2 = props.property === undefined ? 'defaultValue' : props.property; const var3 = props.property ? props.property.subproperty : 'defaultValue'; ``` with: ```ts const var = props.property ?? 'defaultValue'; const var2 = props.property ?? 'defaultValue'; const var3 = props.property?.subproperty ?? 'defaultValue'; ``` It's a slightly more concise way of writing the logic which takes advantage of TS native operators. I basically did a regex search and replaced obvious logic. There are a couple modules where the logic itself was unnecessarily redundant, which I replaced. For example: https://github.com/aws/aws-cdk/blob/31132caec4565735d2ccc32b43f078d634a5ca43/packages/%40aws-cdk/aws-ecs/lib/base/base-service.ts#L217-L218 Can be replaced by: ```ts const port = props.port ?? (protocol === elbv2.ApplicationProtocol.HTTPS ? 443 : 80); ``` Because the check for ```protocol === undefined``` setting the default of port 80 can be rolled into the check for ```protocol === elbv2.ApplicationProtocol.HTTPS``` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…coalescing operators (aws#12277) This is an unnecessary refactor that shrinks logic across CDK, mostly by replacing phrases like: ```ts const var = props.property !== undefined ? props.property : 'defaultValue'; const var2 = props.property === undefined ? 'defaultValue' : props.property; const var3 = props.property ? props.property.subproperty : 'defaultValue'; ``` with: ```ts const var = props.property ?? 'defaultValue'; const var2 = props.property ?? 'defaultValue'; const var3 = props.property?.subproperty ?? 'defaultValue'; ``` It's a slightly more concise way of writing the logic which takes advantage of TS native operators. I basically did a regex search and replaced obvious logic. There are a couple modules where the logic itself was unnecessarily redundant, which I replaced. For example: https://github.com/aws/aws-cdk/blob/31132caec4565735d2ccc32b43f078d634a5ca43/packages/%40aws-cdk/aws-ecs/lib/base/base-service.ts#L217-L218 Can be replaced by: ```ts const port = props.port ?? (protocol === elbv2.ApplicationProtocol.HTTPS ? 443 : 80); ``` Because the check for ```protocol === undefined``` setting the default of port 80 can be rolled into the check for ```protocol === elbv2.ApplicationProtocol.HTTPS``` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This is an unnecessary refactor that shrinks logic across CDK, mostly by replacing phrases like:
with:
It's a slightly more concise way of writing the logic which takes advantage of TS native operators. I basically did a regex search and replaced obvious logic.
There are a couple modules where the logic itself was unnecessarily redundant, which I replaced. For example:
aws-cdk/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
Lines 217 to 218 in 31132ca
Can be replaced by:
Because the check for
protocol === undefinedsetting the default of port 80 can be rolled into the check forprotocol === elbv2.ApplicationProtocol.HTTPSBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license