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

Use basepath in s3 assets #2265

Merged
merged 1 commit into from Jan 19, 2022
Merged

Use basepath in s3 assets #2265

merged 1 commit into from Jan 19, 2022

Conversation

TowardsDeath
Copy link
Contributor

I ran into the problem described in #1620: when using a basePath in the Next.js application, the S3 asset bucket remains empty. There was a fix mentioned in the issue, but no PR yet, so I've taken the liberty to make that change and open this PR. I tried the change locally and it worked.

@dphang or any other maintainer: I'm of course willing to add tests, but I couldn't figure out how to integrate such a basePath test with the existing CDK tests.

@slsnextbot
Copy link
Collaborator

Handler Size Report

No changes to handler sizes.

Base Handler Sizes (kB) (commit aab033f)

{
    "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": 1497,
            "Minified": 807
        },
        "Regeneration Lambda": {
            "Standard": 1184,
            "Minified": 545
        },
        "Regeneration Lambda V2": {
            "Standard": 1254,
            "Minified": 573
        }
    }
}

New Handler Sizes (kB) (commit b36be83)

{
    "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": 1497,
            "Minified": 807
        },
        "Regeneration Lambda": {
            "Standard": 1184,
            "Minified": 545
        },
        "Regeneration Lambda V2": {
            "Standard": 1254,
            "Minified": 573
        }
    }
}

@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #2265 (b36be83) into master (aab033f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
+ Coverage   83.51%   83.54%   +0.03%     
==========================================
  Files         102      102              
  Lines        3669     3671       +2     
  Branches     1167     1169       +2     
==========================================
+ Hits         3064     3067       +3     
+ Misses        593      592       -1     
  Partials       12       12              
Impacted Files Coverage Δ
...rless-components/nextjs-cdk-construct/src/index.ts 93.39% <100.00%> (+0.12%) ⬆️
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 aab033f...b36be83. Read the comment docs.

@dphang
Copy link
Collaborator

dphang commented Jan 19, 2022

I think this is fine as it's a small enough change. Thx!

@dphang dphang merged commit cf780e9 into serverless-nextjs:master Jan 19, 2022
@moshfrid
Copy link
Contributor

i dont know why, but using this new alpha version seems to break isr.
image

@dphang
Copy link
Collaborator

dphang commented Jan 22, 2022 via email

@moshfrid
Copy link
Contributor

To be honest Im very confused, here's the function that's failing
image

@TowardsDeath
Copy link
Contributor Author

I've had this error as well with Next, even before this change, so my guess is that it's not serverless-nextjs' fault. I haven't yet investigated what could be the cause.

@moshfrid
Copy link
Contributor

@DoNormal I'm not having any issues using the alpha.4 tho

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.

None yet

4 participants