feat(lambda-at-edge): support trailing slash / non-trailing slash redirects #556

dphang · 2020-08-22T07:06:27Z

Description

This adds support for server-side redirects (via Lambda origin request handler) based on value of trailingSlash in next.config.js, or by default trailingSlash = false if not set.

By putting it in origin request handler (instead of viewer request handler), we ensure this code is not called on every request, only for redirection requests / requests that were not cached by CloudFront. Also, we do not do regex matching for perf/security reasons, although I believe this is how Next.js does it (e.g for files: vercel/next.js#14681).

Here's the feature from Next.js: https://nextjs.org/docs/api-reference/next.config.js/trailing-slash. I saw a few issues about this, such as #479 but didn't see anyone was working on this for a while, and I had some free time, so I thought I'd take a stab at it. Hopefully no one else has already been working on it!

SSR/SSG pages: if trailingSlash is set to true, then URLs like example.com/mypage get redirected to example.com/mypage/.
SSR/SSG pages: if trailingSlash is set to false or not set (i.e default behavior), then URLs like example.com/mypage/ get redirected to example.com/mypage.
Query string is also passed along: e.g if trailingSlash is true then example.com/mypage?abc=123 is redirected to example.com/mypage/?abc=123. Vice-versa if trailingSlash is false.
In addition, data requests (JSON) and public files always get redirected to non-trailing slash URL.

Note: all the above is the same behavior as in Next/Vercel. The redirect uses a 308 status code (Permanent Redirect, same as Next/Vercel. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308. It's supported by all browsers except IE11 pre-windows 10. For full IE11 support, we also always add a refresh header e.g 0;url=/ as per https://nextjs.org/docs/api-reference/next.config.js/redirects / https://github.com/vercel/next.js/pull/10042/files)

The one exception: files in /_next/static are not redirected if they have trailing slash in URL, they will get 403 response code instead. See notes below, this seems due to the separate CloudFront/S3 behavior (Vercel/Next handles this).

Tests

I've updated unit tests in build.test.ts, default-handler.test.ts and default-handler-with-basepath.test.ts. I didn't want to duplicate a bunch of test code by making new files, so I reorganized some test structure and used Jest's describe.each to run the tests where trailingSlash = true and trailingSlash = false. This doubles the number of unit tests in default-handler test files as a result (but still runs fairly quickly). Please me know if there is a better/preferred way to do this.
AWS testing: deployed both with and without trailingSlash=true and tested all cases (SSG, SSR, data files, public files). Note that files in /_next/static get an access denied when using a trailing slash, but it should because they have a separate CloudFront behavior for those S3 files. (Note: Vercel actually handles these redirects as well. Not sure if S3 supports removing trailing slash? But probably not very important, as no one will likely access these files directly.)

Test App

You can try it out here. I set trailingSlash = true, so all page URLs will redirect to the trailing slash version.

App link: https://dpjozqtv88tg7.cloudfront.net (this is my own AWS account, so I will take this down shortly after the PR is done)

SSR:

https://dpjozqtv88tg7.cloudfront.net/anotherSSR (will 308 redirect to trailing slash URL)
https://dpjozqtv88tg7.cloudfront.net/anotherSSR/

SSG:

https://dpjozqtv88tg7.cloudfront.net/anotherSSG (will 308 redirect to trailing slash URL)
https://dpjozqtv88tg7.cloudfront.net/anotherSSG/

Public File (always redirect to non-trailing slash URL, no matter value of trailingSlash):

https://dpjozqtv88tg7.cloudfront.net/app-store-badge.png
https://dpjozqtv88tg7.cloudfront.net/app-store-badge.png/ (will 308 redirect to non-trailing slash URL)

Data File (always redirect to non-trailing slash URL, no matter value of trailingSlash):

https://dpjozqtv88tg7.cloudfront.net/_next/data/uK1LIjhmO7vrJawGZK4LM/anotherSSG.json
https://dpjozqtv88tg7.cloudfront.net/_next/data/uK1LIjhmO7vrJawGZK4LM/anotherSSG.json/ (will 308 redirect to non-trailing slash URL)

Passing query strings:

https://dpjozqtv88tg7.cloudfront.net/anotherSSG?a=123 -> will be redirected to https://dpjozqtv88tg7.cloudfront.net/anotherSSG/?a=123

I'll try to test other types of pages as well when I have some time.

Client (Static) files do not redirect, but will throw a 403 if you try to access with trailing slash (Next/Vercel handles this). This will return a 403:

https://dpjozqtv88tg7.cloudfront.net/_next/static/chunks/main-08b37df8814a5bcd6e19.js/

You can compare the behavior to a Vercel app here: https://nextjs-repros.vercel.app/

The app code is here: https://github.com/dphang/nextjs-repros/tree/343454e1e1c38fc773d3ba4c45dd2d2bac4200dd

…irects

packages/libs/lambda-at-edge/src/build.ts

...ge/tests/integration/with-trailing-slash-next-config/with-trailing-slash-next-config.test.ts

packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts

danielcondemarin · 2020-08-26T11:36:56Z

Cheers for PR'ing @dphang 👍 I'll take a look at this tonight.

danielcondemarin · 2020-08-26T16:32:17Z

packages/libs/lambda-at-edge/src/build.ts

+    const nextConfigPath = path.join(this.nextConfigDir, "next.config.js");
+
+    if (await fse.pathExists(nextConfigPath)) {
+      const nextConfig = await import(nextConfigPath);


@dphang Not all next.js configs would work with this. next.config.js can export an object { ... } , a Promise, or a function. Take a look at https://github.com/serverless-nextjs/serverless-next.js/blob/master/packages/libs/lambda-at-edge/src/lib/createServerlessConfig.ts#L10

Ah I see, thanks! I'll update this.

I updated the code to handle next.config.js exporting a function. However I could not export a Promise as when doing Next.js build as part of the integ test, it gets an error: https://github.com/vercel/next.js/blob/master/errors/promise-in-next-config.md. Here's their related PR from Feb 2019: vercel/next.js#6476. I don't think a Promise export is supported by Next.js?

Also did not see any existing tests where next.config.js exported a Promise, so not sure if this was ever actually supported/tested. @danielcondemarin do you have any example?

The code example I sent I actually took from https://github.com/vercel/vercel/blob/master/packages/now-next/src/create-serverless-config.ts. Maybe it used to be supported at some point and can be removed.

Since I took the code from next's repo I only covered a few base cases (https://github.com/serverless-nextjs/serverless-next.js/tree/master/packages/libs/lambda-at-edge/tests/integration) but not a Promise.

I see, makes sense! Thanks.

packages/libs/lambda-at-edge/src/default-handler.ts

danielcondemarin · 2020-08-26T16:43:14Z

@dphang Looks really good. Thanks for detailing every step. I'm happy to merge once handling the different types of next.config is resolved 👍

…ction

dphang · 2020-08-26T20:01:32Z

@dphang Looks really good. Thanks for detailing every step. I'm happy to merge once handling the different types of next.config is resolved 👍

Thanks for spending the time to review!

marcin-piela-sf · 2020-08-27T16:16:15Z

When can we expect it in alpha version? :)

danielcondemarin · 2020-08-27T18:37:24Z

When can we expect it in alpha version? :)

Just published it as @sls-next/serverless-component@1.17.0-alpha.7. I've not tested it myself, please feedback if you find any issues 🙏

dphang · 2020-08-27T18:44:12Z

When can we expect it in alpha version? :)

Just published it as @sls-next/serverless-component@1.17.0-alpha.7. I've not tested it myself, please feedback if you find any issues 🙏

Thanks! Yes, please try it out and let me know if there are issues. I've tested it with trailingSlash true on a test app (in the PR) and trailingSlash false (on my own app) and had no issues so far. Though I don't have a machine to test redirects with IE11 on Windows 7/8, but it's using the same refresh header as used by Vercel/Next so I assume it works. If someone can help test, that'd be great :)

dphang · 2020-08-28T01:22:42Z

I did find a bug in this PR that breaks the entire deployment for apps using next-compose-plugins (and other plugins that does not handle having undefined default config) no matter if you are using trailingSlash config or not. Sorry that I did not catch this earlier.

Fixed it in this PR: #569

marcin-piela-sf · 2020-08-28T11:29:54Z

I see one problem after deploy, I have a subPath "test/dwa" and app returns 404 on:

https://d1g4p9lwi1bqdw.cloudfront.net/test/dwa
But it works on:
https://d1g4p9lwi1bqdw.cloudfront.net/test/dwa/

I have "trailingSlash:false" in next.js config.

feat(lambda-at-edge): support trailing slash / non-trailing slash red…

190f02d

…irects

dphang mentioned this pull request Aug 22, 2020

Trailing slashes on root pages return 404 #479

Closed

dphang commented Aug 22, 2020

View reviewed changes

packages/libs/lambda-at-edge/src/build.ts Show resolved Hide resolved

dphang commented Aug 22, 2020

View reviewed changes

...ge/tests/integration/with-trailing-slash-next-config/with-trailing-slash-next-config.test.ts Outdated Show resolved Hide resolved

dphang mentioned this pull request Aug 22, 2020

Next 9.5 redirection features not working #541

Closed

dphang commented Aug 22, 2020

View reviewed changes

packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts Show resolved Hide resolved

dphang added 3 commits August 24, 2020 01:33

fix tests and add refresh header for IE11 compatibility

2ecb324

clean up code

404d0ae

Merge branch 'master' into redirects-support

eeacd44

danielcondemarin reviewed Aug 26, 2020

View reviewed changes

packages/libs/lambda-at-edge/src/default-handler.ts Show resolved Hide resolved

danielcondemarin reviewed Aug 26, 2020

View reviewed changes

packages/libs/lambda-at-edge/src/default-handler.ts Show resolved Hide resolved

dphang added 2 commits August 26, 2020 12:52

clean up code and handle case where next.config.js could export a fun…

0590c61

…ction

remove unused import

bdec6c2

add more test coverage

5db33b8

danielcondemarin approved these changes Aug 26, 2020

View reviewed changes

danielcondemarin merged commit ca63b80 into serverless-nextjs:master Aug 26, 2020

dphang deleted the redirects-support branch August 27, 2020 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda-at-edge): support trailing slash / non-trailing slash redirects #556

feat(lambda-at-edge): support trailing slash / non-trailing slash redirects #556

dphang commented Aug 22, 2020 •

edited

danielcondemarin commented Aug 26, 2020

danielcondemarin Aug 26, 2020 •

edited

dphang Aug 26, 2020

dphang Aug 26, 2020 •

edited

dphang Aug 26, 2020

danielcondemarin Aug 26, 2020 •

edited

dphang Aug 26, 2020

danielcondemarin commented Aug 26, 2020

dphang commented Aug 26, 2020

marcin-piela-sf commented Aug 27, 2020

danielcondemarin commented Aug 27, 2020

dphang commented Aug 27, 2020 •

edited

dphang commented Aug 28, 2020 •

edited

marcin-piela-sf commented Aug 28, 2020

feat(lambda-at-edge): support trailing slash / non-trailing slash redirects #556

feat(lambda-at-edge): support trailing slash / non-trailing slash redirects #556

Conversation

dphang commented Aug 22, 2020 • edited

Description

Tests

Test App

danielcondemarin commented Aug 26, 2020

danielcondemarin Aug 26, 2020 • edited

Choose a reason for hiding this comment

dphang Aug 26, 2020

Choose a reason for hiding this comment

dphang Aug 26, 2020 • edited

Choose a reason for hiding this comment

dphang Aug 26, 2020

Choose a reason for hiding this comment

danielcondemarin Aug 26, 2020 • edited

Choose a reason for hiding this comment

dphang Aug 26, 2020

Choose a reason for hiding this comment

danielcondemarin commented Aug 26, 2020

dphang commented Aug 26, 2020

marcin-piela-sf commented Aug 27, 2020

danielcondemarin commented Aug 27, 2020

dphang commented Aug 27, 2020 • edited

dphang commented Aug 28, 2020 • edited

marcin-piela-sf commented Aug 28, 2020

dphang commented Aug 22, 2020 •

edited

danielcondemarin Aug 26, 2020 •

edited

dphang Aug 26, 2020 •

edited

danielcondemarin Aug 26, 2020 •

edited

dphang commented Aug 27, 2020 •

edited

dphang commented Aug 28, 2020 •

edited