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

Support AWS CDK V2 #2187

Merged
merged 13 commits into from
Dec 22, 2021
Merged

Conversation

n04h
Copy link
Contributor

@n04h n04h commented Dec 6, 2021

@slsnextbot
Copy link
Collaborator

slsnextbot commented Dec 6, 2021

Handler Size Report

No changes to handler sizes.

Base Handler Sizes (kB) (commit 7dc3544)

{
    "Lambda": {
        "Default Lambda": {
            "Standard": 1525,
            "Minified": 668
        },
        "Image Lambda": {
            "Standard": 1489,
            "Minified": 802
        }
    },
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1534,
            "Minified": 674
        },
        "Default Lambda V2": {
            "Standard": 1527,
            "Minified": 670
        },
        "API Lambda": {
            "Standard": 634,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1496,
            "Minified": 807
        },
        "Regeneration Lambda": {
            "Standard": 1184,
            "Minified": 545
        },
        "Regeneration Lambda V2": {
            "Standard": 1254,
            "Minified": 573
        }
    }
}

New Handler Sizes (kB) (commit c812ca3)

{
    "Lambda": {
        "Default Lambda": {
            "Standard": 1525,
            "Minified": 668
        },
        "Image Lambda": {
            "Standard": 1489,
            "Minified": 802
        }
    },
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1534,
            "Minified": 674
        },
        "Default Lambda V2": {
            "Standard": 1527,
            "Minified": 670
        },
        "API Lambda": {
            "Standard": 634,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1496,
            "Minified": 807
        },
        "Regeneration Lambda": {
            "Standard": 1184,
            "Minified": 545
        },
        "Regeneration Lambda V2": {
            "Standard": 1254,
            "Minified": 573
        }
    }
}

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #2187 (c812ca3) into master (7dc3544) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2187      +/-   ##
==========================================
+ Coverage   83.51%   83.53%   +0.02%     
==========================================
  Files         102      102              
  Lines        3669     3669              
  Branches     1166     1166              
==========================================
+ Hits         3064     3065       +1     
+ Misses        593      592       -1     
  Partials       12       12              
Impacted Files Coverage Δ
...rless-components/nextjs-cdk-construct/src/index.ts 93.26% <100.00%> (ø)
packages/libs/core/src/images/imageOptimizer.ts 80.17% <0.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dc3544...c812ca3. Read the comment docs.

@n04h n04h marked this pull request as ready for review December 6, 2021 15:47
@n04h
Copy link
Contributor Author

n04h commented Dec 11, 2021

@dphang
Could you review, please?

@dphang
Copy link
Collaborator

dphang commented Dec 13, 2021

Thanks! I think it generally looks fine but just wondering is it backwards compatible for those upgrading from CDK v1 (or at least easy to upgrade)? Although maybe since it is marked as experimental anyway it should be fine to make slight breaking changes.

@n04h
Copy link
Contributor Author

n04h commented Dec 13, 2021

@dphang
Thank you for confirming!
The major change is to unify the AWS Construct Library into a single package.
I think so it is generally as backwards compatible.

@dphang
Copy link
Collaborator

dphang commented Dec 15, 2021

LGTM, can you resolve the lockfile conflict first (guess there may have been some automated dependency updates)

Copy link

@shishkin shishkin left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. I've tested it and it works 🥇

"@sls-next/cdk-construct": "^3.2.0",
"@sls-next/cdk-construct": "link:../../",
"aws-cdk-lib": "2.0.0",
"constructs": "10.0.0",

Choose a reason for hiding this comment

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

May I wish the versions not be pinned to the patch level? Otherwise I'd have to override dependencies to overcome TypeScript strict errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shishkin
Fixed at 0a62b3c. Could you please confirm that I am not mistaken?

@n04h n04h force-pushed the feature/update-to-aws-cdk-v2 branch 2 times, most recently from 2a9355d to 6638e62 Compare December 15, 2021 15:22
@n04h n04h force-pushed the feature/update-to-aws-cdk-v2 branch from 6638e62 to 9f53657 Compare December 15, 2021 15:25
@n04h n04h force-pushed the feature/update-to-aws-cdk-v2 branch 2 times, most recently from 519de63 to 0a62b3c Compare December 15, 2021 16:00
@n04h
Copy link
Contributor Author

n04h commented Dec 15, 2021

@dphang
CI did not pass due to AWS failure...
Can you please re-run it?

I've made a new commit, so I no longer need you to respond to it!

image

"@aws-cdk/core": "1.134.0"
"aws-cdk": "2.0.0",
"aws-cdk-lib": "2.0.0",
"constructs": "10.0.0"

Choose a reason for hiding this comment

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

Sorry @n04h , I actually meant these package dependencies, not the dependencies of the example project which I commented about. I see those peer dependencies were pinned before your change. I think for TypeScript users it would make sense to allow patch (~) if not minor (^) version upgrades.

@shishkin
Copy link

@n04h I've played around with cdk dependencies in my local fork and here is what I think might be the optimal configuration: shishkin@cbe4ff4#diff-31ab403361720bb5176a7892a6983567ec894b97dec186abeadd7a3ba17c9082.
Note that I've removed aws-cdk from dependencies, since it's only needed in development and is already in devDependencies.
Also note the updated usage of removed aws-cdk-lib/core module in shishkin@b0c947f#diff-88d867930e35085285ebad2ea80ceb765fab4542fb6faa05f84322e368c68134.

Feel free to cherry pick my commits in your PR and thanks for moving this forward!

@n04h
Copy link
Contributor Author

n04h commented Dec 16, 2021

@shishkin
Thanks commits and review!
I cherry-picked and committed it.

@n04h n04h changed the title chore(nextjs-cdk-construct): update dependency aws cdk to v2 Support AWS CDK V2 Dec 19, 2021
@dphang
Copy link
Collaborator

dphang commented Dec 22, 2021

LGTM, thanks! I'll merge it now to avoid any dependency conflicts, if any updates needed feel free to raise another PR

@dphang dphang merged commit b329326 into serverless-nextjs:master Dec 22, 2021
@n04h n04h deleted the feature/update-to-aws-cdk-v2 branch December 22, 2021 02:07
@kiernan kiernan mentioned this pull request Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support AWS CDK V2
4 participants