serverless-next.js: Conflicting rewrites #2 - Incorrect paths in Preview Mode (1.18.0-alpha.16)

New issue per maintainer admonishments. This is continued from https://github.com/serverless-nextjs/serverless-next.js/issues/696, same setup as there (@dphang).

Describe the bug

When using top-level rewrite (per past issues – see rewrites / repo below), upon visiting a route in preview mode, the incorrect URL is passed to getStaticProps (via params). Issue goes away if rewrites are removed, or if not in preview mode.

Actual behavior

URL slug passed in params to getStaticProps is incorrect.

Expected behavior

URL slug passed in params to getStaticProps is correct.

Steps to reproduce

Deploy my example
Visit this URL: /api/preview?secret=96ffe7d4-2ad2-4699-b64a-a5f74750962e&slug=/dynamic-1
- Either check logs, or simply look at the page. The params key incorrectly has part of the rewrite destination in it.
For added fun, visit the non-rewritten URL for the same page /api/preview?secret=96ffe7d4-2ad2-4699-b64a-a5f74750962e&slug=/content/dynamic-1
- The app now thinks we’re at /content/content/dynamic-1

Screenshots/Code/Logs

Reproduction: https://github.com/patricktyndall/sls-next-bug-reproduction
The rewrite in question:

[{
	source: '/:path*',
	destination: '/content/:path*',
}]

Versions

OS/Environment: Mac
@sls-next/serverless-component version: 1.18.0-alpha.16
Next.js version: 9.5.5

Additional context

~If you visit /content/dynamic-1 not in preview mode, you get a 404. This is also a bug, and is actually a regression added in alpha.16.~

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 15

Most upvoted comments

PR for (1): https://github.com/serverless-nextjs/serverless-next.js/pull/719

dphang on Oct 28, 2020

That’s what I thought too, but from my testing it seemed that Next.js server skips rewrites only for non-dynamic pages and public files

Good catch on this, that’s definitely not intuitive. I would have thought to skip rewrites for anything with a non-dynamic segment. (I wonder if this holds for non-catch-all dynamic segments / rewrites). I guess this is the utility of the “no-op”; that’s not explained well in the next docs.

patricktyndall on Oct 22, 2020

Clarification here - since /content/[...all] is a path segment defined via pages/, the correct logic would be to skip the rewrite on a request for /content/dynamic-1 since it matches a pages/ pattern, which have higher precedence than rewrites. (I think we’re on the same page about this – I’m not familiar with path-to-regexp but wanted to confirm that logic.)

That’s what I thought too, but from my testing it seemed that Next.js server skips rewrites only for non-dynamic pages and public files. So SSR dynamic pages as well as SSG dynamic pages (using getStaticPaths, such as /content/[...all]) will get rewritten unless you explicitly stop it using the workaround I mentioned. This seemed to be why I was getting a 404 with that :path -> content/:path rewrite.

The relevant change is here: https://github.com/serverless-nextjs/serverless-next.js/pull/700/files, this actually brings it to parity with Next.js.

So I would rephrase the logic as: “When rewriting, destination segments that were not matched by the dynamic route segment ([…all]) should not passed as params”

Thanks for this - yeah, this is still strange to me but if that’s what Next.js does, then we should have parity with it.

I’ll leave open to think how to do this, considering your feedback. Let me know if you have any other thoughts on this. @danielcondemarin FYI

dphang on Oct 22, 2020