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

Dynamic routes not working #16

Closed
andrewgadziksonos opened this issue Nov 27, 2020 · 10 comments
Closed

Dynamic routes not working #16

andrewgadziksonos opened this issue Nov 27, 2020 · 10 comments
Assignees
Labels
bug Something isn't working
Projects

Comments

@andrewgadziksonos
Copy link

andrewgadziksonos commented Nov 27, 2020

Hello!

I'm seeing issues with dynamic pages and I just wanted to make sure that it's not something I'm doing incorrectly. Here's my tf-next build output,

image

You can see that I have a mix of dynamic routes, catch-all routes and optional catch-all routes.

Going to https://d1hjov1nk12qxr.cloudfront.net/products/one results in a 404, but going to https://d1hjov1nk12qxr.cloudfront.net/products/[pid] results in rendered HTML.

Here's a zip of my test repo,

nextjs-aws-demo.zip

Thanks for the help in advance,

Andrew

@andrewgadziksonos
Copy link
Author

I realized that I forgot to add a next.config.js file to the repo, after adding the following,

module.exports = {
  // Build the server for a serverless environment
  target: 'serverless',
};

and rebuilding, I'm still seeing the same errors

@ofhouse
Copy link
Member

ofhouse commented Nov 28, 2020

Thanks for creating & posting that example.
Will take a deeper look tomorrow. Could be possible that something broke with the latest Next.js 10 release, we haven't upgraded yet.

@ofhouse ofhouse self-assigned this Nov 28, 2020
@ofhouse ofhouse added the bug Something isn't working label Nov 28, 2020
@ofhouse ofhouse added this to To do in Tasks Nov 28, 2020
@andrewgadziksonos
Copy link
Author

I’ll also try and downgrade to 9.5 later and see if that fixes it. Thanks!

@andrewgadziksonos
Copy link
Author

Hmm - That didn't seem to help. Using next@9.5.5 still produces 404s - no errors in the logs. Let me know if you need anything else from me.

@ofhouse ofhouse moved this from To do to In progress in Tasks Nov 30, 2020
@ofhouse
Copy link
Member

ofhouse commented Nov 30, 2020

Sorry for the delay. Checked it out and got the same result, so routing is definitely broken for this example.
Currently gearing up a better testing solution for real world examples which I expect to ship by tomorrow and then I can take a better look what happened in this case.

@andrewgadziksonos
Copy link
Author

No worries, and sounds good. I'm happy to help debug if needed

@ofhouse
Copy link
Member

ofhouse commented Dec 8, 2020

Short update regarding this issue:
I've tracked the issue down to this code:
https://github.com/dealmore/terraform-aws-next-js/blob/main/packages/proxy/src/proxy.ts#L92

This was a workaround introduced for slug routes (e.g. /some-page/[...slug]) that we mostly rely on for our own projects.
While it works for slug-routes it breaks dynamic routing (e.g. /some-page/[id]) at this point because it cuts the arguments from the URL and prevents the dynamic part from being passed to the lambda.

To prevent the issue in the future we set up an local e2e testing (proxy -> lambda) in #17 powered by AWS SAM and introduce further unit tests to cover that use cases.

Routing is a bit complicated here because it is mostly based on reverse engineering Vercel's proxy solution but with the testing we should have a solution ready soon.

@andrewgadziksonos
Copy link
Author

Thanks @ofhouse for the update! Looking forward to the fix. Let me know how I can help

@ofhouse ofhouse linked a pull request Dec 19, 2020 that will close this issue
4 tasks
@ofhouse
Copy link
Member

ofhouse commented Dec 19, 2020

@andrewgadziksonos ok, end of the year was busier than I thought, so sorry again for the delay on this issue 🙈.

I finally implemented a working solution for the issue and the e2e tests results also confirm that it is working.
I track the current status in #17 and when CI is green we are ready for a new release 😬

Unfortunately there is still one test failing for a fallback route.
Since fallback routes didn't work before either I think this is not necessarily linked to this issue and we may defer support for fallback routes in a feature request.
Will make decision on this tomorrow and then I will publish a new release.

@ofhouse ofhouse removed a link to a pull request Dec 19, 2020
4 tasks
ofhouse added a commit that referenced this issue Dec 21, 2020
Pull-Request: #17
Relates to issue #16
@ofhouse
Copy link
Member

ofhouse commented Dec 22, 2020

Good news, we finally have a new v0.2.0 release out that should fix the issue.

Please note that you also need to update the @dealmore/terraform-next-build in your package.json to 0.2.0.

@ofhouse ofhouse closed this as completed Jan 9, 2021
@ofhouse ofhouse moved this from In progress to Done in Tasks Jan 9, 2021
@qpre qpre mentioned this issue Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Tasks
  
Done
Development

No branches or pull requests

2 participants