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

[patricktyndall]

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.~

[dphang]

Thanks for creating the new issue - I thought it was related to just preview mode, not rewrites.

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.

In the local next server (yarn build && yarn start), http://localhost:3000/content/dynamic-1 is also returning a 404 as well for me, when the rewrite below is added as you mentioned:

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

It seems to be the same behavior as serverless-next.js, so it seems the change is in line with this. I suspect Next.js is rewriting /content/dynamic-1 to /content/content/dynamic-1 which is not found in non-preview mode as fallback is false. Can you please confirm this?

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

Yeah, this should be the same root cause - when including above rewrite, in the current implementation, the rewrite will map from content/dynamic-1 -> content/content/dynamic-1 because of the rewrite /:path* -> /content/:path*, :path matches content/dynamic-1. This should be the behavior for path-to-regexp. Not quite sure why Next.js is deviating from this?

I guess to fix this, when rewriting we need make sure only the :path part is passed into dynamic routes? Though personally, this is really strange behavior from Next.js.

A workaround I can think of would be to add a rewrite from /content/:path* to /content/:path* at the start.

[patricktyndall]

is also returning a 404 ... Can you please confirm this?

I can confirm. That's my mistake -- previously I had the additional "no-op" rewrite, which must have made next respond to the non-rewritten path as well. TIL. Edited the issue to exclude this.

I suspect Next.js is rewriting /content/dynamic-1 to /content/content/dynamic-1

This is incorrect. Per next docs, rewrite step is skipped altogether if a /pages pattern is matched. I verified this by setting fallback: true and visiting /content/dynamic-1 -- only dynamic-1 is passed.

• Although this does not explain why /content/dynamic-1 404s in the fallback: false case. It may be simply that next 404's when you visit a rewrite destination via its non-rewritten path, but I've been unable to find confirmation for this.

This should be the behavior for path-to-regexp. Not quite sure why Next.js is deviating from this?

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.)

I guess to fix this, when rewriting we need make sure only the :path part is passed into dynamic routes?

This is mostly correct, but I think the logic is slightly more subtle. Consider the following redirect setup:

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

Using next server, visiting /dynamic-1 ends up calling getStaticProps (in /pages/content/[...all]) with { all: [ 'random', 'segment', 'dynamic-1' ] }. So it's not just the :path* part that should be passed, but rather any segment in the destination that matches the catch-all.

• (So if I moved my page to /pages/content/random/[...all], I would only expect to receive segment and dynamic-1 for the above request + rewrites)

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"

Mind you, I have no idea why the above setup would be useful, but next supports it so IMO this should too.

A workaround I can think of would be to add a rewrite from /content/:path* to /content/:path* at the start.

Yeah, this was my original workaround discussed in previous iterations. It makes sense but I don't think it should be necessary, if the goal is compatibility with next

[dphang]

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

[patricktyndall]

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]

Hey @dphang can you inform on the priority of this issue / when we can expect a fix?

Preview mode is a requirement for my current project so this is blocking. Since #646 is fixed, I'm considering abandoning rewrites altogether and using the catch-all approach (but would rather not, if the current issue will be fixed soon). Thanks again for your work.

[dphang]

I have not continued looking at it besides the last discussions. I will try to take a look on or by the weekend. If you need it sooner, please feel free to suggest an implementation or PR and I'll be happy to review. Thanks.

[patricktyndall]

Ok thanks. Unfortunately I will not be able to take a look myself (wish I could). Good luck on the implementation!

[dphang]

Had a quick test again:

1. I think rewrites are working as expected, except for 1 more bug I found, it should not try to rewrite _next/data requests. If you have rewrite from /:path to somewhere else it conflicts with this. Will make a fix for this.
2. The content/blah coming in all for fallback: true seems to be coming from the data request rendering at runtime (since it's not statically generated at build time for blah (only dynamic-1, dynamic-2), it uses the JS file to generate the page data JSON). It seems to generate something like this:

{"pageProps":{"all":"content/blah"},"__N_SSG":true}

whereas the expected JSON should be:

{"pageProps":{"all":"blah"},"__N_SSG":true}

I will investigate more on why this JS (generated by Next.js build) is not matching blah correctly to all.

[patricktyndall]

Cheers, thanks for testing it out. LMK when you have a fix for 1), that seems important. (I'm not using fallback:true, so 2) is less important to me personally, but I'm really glad you tested it).

[dphang]

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

[dphang]

@patricktyndall can you please check if your specific use cases are working now with the latest alpha version or if there are other bugs.

Meanwhile I will take a look at the fallback: true fix for its dynamic fallback's data requests. It seems like content/blah may not be matching correctly in the JS because of missing leading forward slash, e.g /content/blah.

[patricktyndall]

My cases are working, thanks!

[dphang]

Thanks for confirming.

I did test issue (2) a bit more, it seems that the Node.js/Express req.url is set to the data request URL e.g the example of /_next/data/<build_id>/content/blah.json. However, the JS file itself for content/[all].js is not parsing the parameters for getStaticProps correctly, it seems to be able to reduce it to content/blah but not blah for the all parameter, which is what is expected. I would have thought Next.js render should be able to render the right JSON. Will need to check if we are using this correctly:

      const { renderOpts } = await page.renderReqToHTML(
        req,
        res,
        "passthrough"
      );

[patricktyndall]

Hey @dphang when I tested this, I tested using a different rewrite config that was also giving issues in preview mode. That's my bad for not testing the one we were discussing. However there are still more general issues with the rewrite config I ultimately want to use, unrelated to preview mode. I created a new issue (#733) and updated the reproduction.