chore(cdk-lib): migrate to jsii@5.0 / jsii-rosetta@5.0 by RomainMuller · Pull Request #24425 · aws/aws-cdk

RomainMuller · 2023-03-02T15:29:10Z

Updates dependencies & code as necessary to use jsii@5.0 to build the AWS CDK.

BREAKING CHANGE: The return type of aws-cdk-lib.aws_ec2.SecurityGroup.determineRuleScope was changed from a tuple ([SecurityGroupBase, string]) to a struct with the same values, because tuple types are not supported over the jsii interoperability layer, but jsii@v1 was incorrectly allowing this to be represented as the JSON primitive type. This made the API unusable in non-JS languages. The type of the metadata property of aws-cdk-lib.aws_s3_deployment.BucketDeploymentProps was changed from an index-only struct to an inline map, because jsii@v1 silently ignored the index signature (which is otherwise un-supported), resulting in an empty object in non-JS/TS languages. As a consequence, the values of that map can no longer be undefined (as jsii does not currently support nullable elements in collections).

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

aws-cdk-automation

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.

RomainMuller

The bulk of code changes here fall into one of these categories:

Promise<void> must be explicitly typed, or TypeScript will require a parameter be provided to the resolve handler.
catch bindings are unknown by default, explicitly typed as any in places where this was assumed, and removed the binding entirely in the few places where it was not used.
abstract async is no longer valid, removed the async keyword (the contract really is just "returns a promise", which is expressed fully with the return type declaration)
Some generic helper methods needed tweaking due to hardened type checking safety around uses of Object.keys, Object.values, Object.entries.
Some places blindly assumed Object.keys returns only string, but it could contain Symbol objects which have a throwing toString (they need to be strigified as String(sym)). The 4.9 TypeScript compiler actually reports this as an error. Hardened such uses.
Adding overrides keywords as I am proposing to enable noImplicitOverride

Then, there are some more annoying aspects where jsii@v4.9 checks (and prohibits) things that v1 silently ignores or mis-interprets (back-porting of these checks to v1 is planned, but they'll likely need to be WARNING there). I am adding a comment on each use-case to facilitate discussion.

packages/@aws-cdk/aws-cognito/lib/user-pool.ts

RomainMuller · 2023-03-02T15:58:04Z

packages/@aws-cdk/aws-ec2/lib/security-group.ts

    connection: Port,
    fromTo: 'from' | 'to',
-    remoteRule?: boolean): [SecurityGroupBase, string] {
+    remoteRule?: boolean): RuleScope {


This is incorrectly processed by jsii@v1, which treats it the same as object, resulting in the JSON primitive type.

The API is protected. Is it intended for external use? Can it be made @internal?

Changing the return type to an object here is a breaking change, but provides better semantics... This API is currently very awkward to use in non-JS/TS languages.

Note - I could not find any use of this API outside of the CDK Construct library on a cursory search on GitHub.

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/types.ts

packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts

tools/@aws-cdk/pkglint/lib/rules.ts

RomainMuller · 2023-03-03T08:43:30Z

scripts/update-dependencies.sh

@@ -1,7 +1,7 @@
 #!/bin/bash
 set -euo pipefail

-export NODE_OPTIONS="--max-old-space-size=4096 ${NODE_OPTIONS:-}"
+export NODE_OPTIONS="--max-old-space-size=8196 ${NODE_OPTIONS:-}"


Uniformed the --max-old-space-size= in all the scripts to the highest value that was in any of them.

I've had jsii run out of heap building aws-cdk-lib here, with ~4G heap (the default on my system). Bumping to 8G fixed that...

We may want to review jsii's memory usage separately to see whether there are avenues for improvement (last similar exercise we did concluded that the bulk of the use was TypeScript itself, but it can't hurt to reassess).

packages/cdk-cli-wrapper/lib/cdk-wrapper.ts

RomainMuller · 2023-03-03T08:55:54Z

packages/@aws-cdk/integ-tests/test/assertions/providers/lambda-handler/base.test.ts

@@ -47,7 +48,7 @@ describe('CustomResourceHandler', () => {
      const nocked = nockUp((body) => {
        return body.Status === 'SUCCESS'
        && body.Reason === 'OK'
-        && body.Data === {}
+        && isDeepStrictEqual(body.Data, {})


The TypeScript 4.9 compiler considers comparing an object literal expression as an error, since this would be a reference comparison, and not a value comparison, and the ECMAScript standard mandates that each individual object literal is an individual object (so such comparisons can never be true).

It is unclear to me how this test could have succeeded. Perhaps a V8-specific quirk?

packages/@aws-cdk/core/lib/private/asset-staging.ts

packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/context-queries.ts

…next

@deprecated

…faces as '@deprecated'

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

Naumel

Relevant: https://github.com/aws/aws-cdk/pull/24425/files?diff=split&w=1#r1134057642, therefore approving √

I am marking the PR as pr/do-not-merge to allow others to review, if they chose to do so.

allowed-breaking-changes.txt

Naumel · 2023-03-21T09:39:44Z

packages/@aws-cdk/aws-apigatewayv2/lib/http/authorizer.ts

 }

-function undefinedIfNoKeys<A>(obj: A): A | undefined {
+function undefinedIfNoKeys<A extends { [key: string]: unknown }>(obj: A): A | undefined {


[breadcrumb] Anything is assignable to unknown, but unknown isn't assignable to anything but itself. Diff from any.

mergify · 2023-03-21T10:00:30Z

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

Co-authored-by: Naumel <104374999+Naumel@users.noreply.github.com>

mergify · 2023-03-22T17:02:51Z

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

mergify · 2023-03-22T18:42:27Z

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 · 2023-03-22T21:08:22Z

AWS CodeBuild CI Report

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

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

mergify · 2023-03-22T21:08:42Z

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

Updates dependencies & code as necessary to use `jsii@5.0` to build the AWS CDK. BREAKING CHANGE: The return type of `aws-cdk-lib.aws_ec2.SecurityGroup.determineRuleScope` was changed from a tuple (`[SecurityGroupBase, string]`) to a struct with the same values, because tuple types are not supported over the jsii interoperability layer, but `jsii@v1` was incorrectly allowing this to be represented as the `JSON` primitive type. This made the API unusable in non-JS languages. The type of the `metadata` property of `aws-cdk-lib.aws_s3_deployment.BucketDeploymentProps` was changed from an index-only struct to an inline map, because `jsii@v1` silently ignored the index signature (which is otherwise un-supported), resulting in an empty object in non-JS/TS languages. As a consequence, the values of that map can no longer be `undefined` (as `jsii` does not currently support nullable elements in collections). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This follows the pattern in aws#24425 which seems to especially happen after aws#24376 when running these scripts. This follows the pattern in `scripts/gen.sh` or `/build.sh`.

…#24905) This follows the pattern in #24425 which seems to especially happen after #24376 when running these scripts. Closes #24932 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

chore: migrate to jsii-compiler 4.9, enable noImplicitOverride

b95dc15

RomainMuller self-assigned this Mar 2, 2023

github-actions bot added the p2 label Mar 2, 2023

aws-cdk-automation requested a review from a team March 2, 2023 15:29

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 2, 2023

aws-cdk-automation previously requested changes Mar 2, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Mar 2, 2023

RomainMuller commented Mar 3, 2023

View reviewed changes

RomainMuller added 6 commits March 3, 2023 10:23

chore: migrate to jsii-compiler 4.9, enable noImplicitOverride

3a8e646

revert noImplicitOverride change

c508bff

Merge remote-tracking branch 'origin/main' into rmuller/compiler-v49-…

14dca9b

…next

Use '@jsii ignore' instead of '@internal', re-introduce deleted inter…

b279987

…faces as '@deprecated'

undo asset staging logging change

23c69bc

remove override keyword

ccde9ae

RomainMuller changed the title ~~chore: migrate to jsii-compiler 4.9, enable noImplicitOverride~~ chore: migrate to jsii-compiler 4.9 Mar 13, 2023

RomainMuller added 8 commits March 13, 2023 15:47

add docstring

9e81dff

fix nullable map value signatures

99d0f97

fix nullable map value

5c8683c

update THIRD_PARTY_LICENSES

12f0507

silence awslint rule about unused type

2150924

ignore one more unused type

2ec7f42

remove last noImplicitOverride

2edabba

allow the necessary breaking changes

eed8bd4

RomainMuller force-pushed the rmuller/compiler-v49-next branch from 640ff29 to eed8bd4 Compare March 14, 2023 08:58

RomainMuller and others added 4 commits March 16, 2023 11:02

Merge branch 'main' into rmuller/compiler-v49-next

40a0bcf

chore: update jsii dependencies from v1 line

b66a525

update THIRD_PARTY_LICENSES file

f93c14c

further upgrade jsii 1.x

b279f89

RomainMuller force-pushed the rmuller/compiler-v49-next branch from f8351c2 to b279f89 Compare March 17, 2023 09:41

switch to jsii @ jsii-rosetta v5.0-next

9bc853d

remove v4.9-next dependencies

d13fa1c

RomainMuller marked this pull request as ready for review March 20, 2023 10:49

RomainMuller requested a review from aws-cdk-automation March 20, 2023 10:49

RomainMuller and others added 2 commits March 20, 2023 11:49

Merge branch 'main' into rmuller/compiler-v49-next

5c7924c

update to GA release of @5.0.x

a19ebdf

RomainMuller changed the title ~~chore: migrate to jsii@5.0 / jsii-rosetta@5.0~~ chore(aws-cdk-lib): migrate to jsii@5.0 / jsii-rosetta@5.0 Mar 20, 2023

RomainMuller changed the title ~~chore(aws-cdk-lib): migrate to jsii@5.0 / jsii-rosetta@5.0~~ chore(cdk-lib): migrate to jsii@5.0 / jsii-rosetta@5.0 Mar 20, 2023

RomainMuller added the pr-linter/exempt-breaking-change The PR linter will not require stability in stable modules label Mar 20, 2023

Merge branch 'main' into rmuller/compiler-v49-next

4306207

Naumel approved these changes Mar 21, 2023

View reviewed changes

Naumel added the pr/do-not-merge This PR should not be merged at this time. label Mar 21, 2023

RomainMuller and others added 2 commits March 21, 2023 15:55

Update allowed-breaking-changes.txt

0d1461c

Co-authored-by: Naumel <104374999+Naumel@users.noreply.github.com>

Merge branch 'main' into rmuller/compiler-v49-next

af5d28e

RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label Mar 22, 2023

mergify bot and others added 2 commits March 22, 2023 17:02

Merge branch 'main' into rmuller/compiler-v49-next

e96bf3e

fix catch binding

d348ef7

Merge branch 'main' into rmuller/compiler-v49-next

61dfab0

mergify bot merged commit 6d581d7 into main Mar 22, 2023

mergify bot deleted the rmuller/compiler-v49-next branch March 22, 2023 21:08

laurelmay mentioned this pull request Apr 2, 2023

chore(scripts): pass higher memory limit in buildup and builddown #24905

Merged

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Conversation

RomainMuller commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

RomainMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RomainMuller Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

RomainMuller Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RomainMuller Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RomainMuller Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Naumel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Naumel Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 21, 2023

Uh oh!

mergify bot commented Mar 22, 2023

Uh oh!

mergify bot commented Mar 22, 2023

Uh oh!

aws-cdk-automation commented Mar 22, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RomainMuller commented Mar 2, 2023 •

edited

Loading