Skip to content
This repository was archived by the owner on Jan 28, 2025. It is now read-only.

fix(core): fix 'basePath: false' external rewrites #1859

Merged
merged 2 commits into from
Oct 18, 2021

Conversation

dphang
Copy link
Collaborator

@dphang dphang commented Oct 18, 2021

Fixes: #1858

@slsnextbot
Copy link
Collaborator

slsnextbot commented Oct 18, 2021

Handler Size Report

There are changes to handler sizes. Please review.

Base Handler Sizes (kB) (commit 4316b18)

{
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1505,
            "Minified": 664
        },
        "Default Lambda V2": {
            "Standard": 1496,
            "Minified": 660
        },
        "API Lambda": {
            "Standard": 631,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1471,
            "Minified": 797
        },
        "Regeneration Lambda": {
            "Standard": 589,
            "Minified": 241
        },
        "Regeneration Lambda V2": {
            "Standard": 655,
            "Minified": 267
        }
    }
}

New Handler Sizes (kB) (commit 79262ce)

{
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1507,
            "Minified": 664
        },
        "Default Lambda V2": {
            "Standard": 1498,
            "Minified": 660
        },
        "API Lambda": {
            "Standard": 632,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1471,
            "Minified": 797
        },
        "Regeneration Lambda": {
            "Standard": 589,
            "Minified": 241
        },
        "Regeneration Lambda V2": {
            "Standard": 655,
            "Minified": 267
        }
    }
}

@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #1859 (76e9c01) into master (4316b18) will decrease coverage by 0.03%.
The diff coverage is 91.37%.

❗ Current head 76e9c01 differs from pull request most recent head 79262ce. Consider uploading reports for the commit 79262ce to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1859      +/-   ##
==========================================
- Coverage   83.81%   83.78%   -0.04%     
==========================================
  Files          99       99              
  Lines        3498     3510      +12     
  Branches     1061     1073      +12     
==========================================
+ Hits         2932     2941       +9     
- Misses        502      505       +3     
  Partials       64       64              
Impacted Files Coverage Δ
packages/libs/core/src/route/page.ts 91.07% <86.48%> (-4.59%) ⬇️
packages/libs/core/src/route/api.ts 100.00% <100.00%> (ø)
packages/libs/core/src/route/basepath.ts 100.00% <100.00%> (ø)
packages/libs/core/src/route/index.ts 100.00% <100.00%> (ø)

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 4316b18...79262ce. Read the comment docs.

@dphang dphang merged commit f60ff4a into master Oct 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the dphang/external-rewrite branch October 18, 2021 08:16
@danjiro
Copy link

danjiro commented Oct 18, 2021

thank you @dphang always for the quick fixes!
Alpha 4 seems to be failing however when building now

error:
  TypeError: Cannot read property 'map' of undefined
    at Object.generate (/home/runner/.serverless/components/registry/npm/@sls-next/[email protected]/node_modules/fast-glob/out/managers/tasks.js:9:37)
    at getWorks (/home/runner/.serverless/components/registry/npm/@sls-next/[email protected]/node_modules/fast-glob/out/index.js:58:29)
    at async (/home/runner/.serverless/components/registry/npm/@sls-next/[email protected]/node_modules/fast-glob/out/index.js:29:17)
    at readDirectoryFiles (/home/runner/.serverless/components/registry/npm/@sls-next/[email protected]/node_modules/@sls-next/core/dist/build/lib/readDirectoryFiles.js:17:42)
    at Builder.readPublicFiles (/home/runner/.serverless/components/registry/npm/@sls-next/[email protected]/node_modules/@sls-next/lambda-at-edge/dist/build.js:60:66)
    at async Builder.build (/home/runner/.serverless/components/registry/npm/@sls-next/[email protected]/node_modules/@sls-next/lambda-at-edge/dist/build.js:375:207)

@dphang
Copy link
Collaborator Author

dphang commented Oct 18, 2021

Ah thanks for pointing out, not sure why it passed in CI but looks like some dependency issue caused by refactoring some build logic into the core package. I will take a look.

EDIT: probably passed in CI because other packages like lambda-at-edge have fast-glob and probably core resolved the dependency from there within the same workspace. But in the distribution since each package is published independently I guess it could not resolve the dependency.

@dphang
Copy link
Collaborator Author

dphang commented Oct 18, 2021

@danjiro should be fixed now

@danjiro
Copy link

danjiro commented Oct 19, 2021

can confirm its working. thanks again @dphang for your awesome work

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrites to external destinations not working
3 participants