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

Fix SSG fallback with basepath #992

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

jvarho
Copy link
Collaborator

@jvarho jvarho commented Apr 14, 2021

Fixes #990

There were two issues:

  1. Page requests happened with basePath in s3Origin.path while data requests had it in request.uri, so normalizeUri broke with fallback pages.
  2. Since basePath starts with / all the S3 operations from response handler broke when using basePath.

@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #992 (108bdb6) into master (72b3c69) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 108bdb6 differs from pull request most recent head 7d385b5. Consider uploading reports for the commit 7d385b5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
+ Coverage   81.51%   81.61%   +0.10%     
==========================================
  Files          69       69              
  Lines        2537     2540       +3     
  Branches      604      602       -2     
==========================================
+ Hits         2068     2073       +5     
  Misses        402      402              
+ Partials       67       65       -2     
Impacted Files Coverage Δ
...ackages/libs/lambda-at-edge/src/default-handler.ts 94.29% <100.00%> (+0.73%) ⬆️

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 72b3c69...7d385b5. Read the comment docs.

@dphang
Copy link
Collaborator

dphang commented Apr 14, 2021

Thanks, I thought these cases covered in the e2e tests but maybe the fallback tests are not working correctly, might be worth trying to take a look at those if you have time too.

Let me run the e2e tests and see if something needs fixing there. (link: https://github.com/serverless-nextjs/serverless-next.js/actions/runs/749314587)

@jvarho
Copy link
Collaborator Author

jvarho commented Apr 14, 2021

I don't think any of the e2e tests combine base path and dynamic paths.

I was going to look into implementing fallback: blocking when I have the time, but it might be worthwhile to write some more tests and refactor the handler first.

@dphang
Copy link
Collaborator

dphang commented Apr 14, 2021

I don't think any of the e2e tests combine base path and dynamic paths.

I was going to look into implementing fallback: blocking when I have the time, but it might be worthwhile to write some more tests and refactor the handler first.

Makes sense, yea we have a separate test package for more dynamic pages tests but that was just without the basepath. Maybe we can just add the test to the next-app-with-basepath package

@dphang dphang merged commit dff598b into serverless-nextjs:master Apr 15, 2021
@jvarho jvarho mentioned this pull request Apr 16, 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.

403 AccessDenied from S3 for dynamic SSG pages when basePath is defined
2 participants