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

Add support for custom headers in custom origins #1448

Merged
merged 7 commits into from
Jul 27, 2021

Conversation

Xaroth
Copy link
Contributor

@Xaroth Xaroth commented Jul 26, 2021

This mostly mimics serverless-components/aws-cloudfront#20 , but then converted to ts.

This adds the 'headers' option for origins, which allows one to set custom headers to be set with these origin requests.

@dphang
Copy link
Collaborator

dphang commented Jul 27, 2021

Thanks, can you just make sure you are formatting the file according to prettier/eslint rules, so the codebase is consistent in style? We have git hooks via husky for that. Please ensure you are running them. Looks like the codacy analysis failed because of those?

EDIT: also it looks like the tests are failing as well. Please fix and I will approve and merge.

@Xaroth
Copy link
Contributor Author

Xaroth commented Jul 27, 2021

Will work on this when I get into the office. I'm not very familiar with the test suite being used (I normally don't develop much js/ts), so this might take me a bit 🙂

@Xaroth
Copy link
Contributor Author

Xaroth commented Jul 27, 2021

@dphang Both points should be fixed. Ironically, the linting issues I already had staged, just not committed, which was a bad on my part. The tests failing was a failure at my end when converting it to ts.

@Xaroth
Copy link
Contributor Author

Xaroth commented Jul 27, 2021

Hm, the test I see failing seems to be related to the fix you just merged in master, so is not related to this patch.

@dphang
Copy link
Collaborator

dphang commented Jul 27, 2021

Hm, the test I see failing seems to be related to the fix you just merged in master, so is not related to this patch.

Yup, I was trying to upgrade a dependency but it failed the build, will revert the change.

Ran e2e tests: https://github.com/serverless-nextjs/serverless-next.js/actions/runs/1070362351

@slsnextbot
Copy link
Collaborator

slsnextbot commented Jul 27, 2021

Handler Size Report

No changes to handler sizes.

Base Handler Sizes (kB) (commit bcc9f73)

{
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1226,
            "Minified": 432
        },
        "API Lambda": {
            "Standard": 86,
            "Minified": 34
        },
        "Image Lambda": {
            "Standard": 894,
            "Minified": 354
        },
        "Regeneration Lambda": {
            "Standard": 620,
            "Minified": 220
        }
    }
}

New Handler Sizes (kB) (commit b3c656c)

{
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1226,
            "Minified": 432
        },
        "API Lambda": {
            "Standard": 86,
            "Minified": 34
        },
        "Image Lambda": {
            "Standard": 894,
            "Minified": 354
        },
        "Regeneration Lambda": {
            "Standard": 620,
            "Minified": 220
        }
    }
}

@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #1448 (09065d3) into master (bcc9f73) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 09065d3 differs from pull request most recent head b3c656c. Consider uploading reports for the commit b3c656c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
+ Coverage   84.14%   84.15%   +0.01%     
==========================================
  Files          96       96              
  Lines        3367     3371       +4     
  Branches      995      996       +1     
==========================================
+ Hits         2833     2837       +4     
  Misses        474      474              
  Partials       60       60              
Impacted Files Coverage Δ
...s-components/aws-cloudfront/src/getOriginConfig.ts 95.83% <100.00%> (+0.83%) ⬆️

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 bcc9f73...b3c656c. Read the comment docs.

@Xaroth
Copy link
Contributor Author

Xaroth commented Jul 27, 2021

I see the e2e runs generated some errors, but I don't think those errors are related to the code, more to the service being used for that test.

@dphang
Copy link
Collaborator

dphang commented Jul 27, 2021

I see the e2e runs generated some errors, but I don't think those errors are related to the code, more to the service being used for that test.

Yea, it's because it was hitting the homepage of https://jsonplaceholder.typicode.com/ for some tests and doing some comparison for rewrites, but because it's HTML and they recently added Cloudflare browser insights, there is some UUID generated that differs on each page load. Fixed it in another PR.

@Xaroth
Copy link
Contributor Author

Xaroth commented Jul 27, 2021

Ah, that does explain a lot then, yeah 🙂

@dphang dphang merged commit dd2fa6e into serverless-nextjs:master Jul 27, 2021
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.

3 participants