Skip to content

Fixed a URL order issue and optimized function for cloudfront #7

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lockehooper
Copy link

Not sure if you still maintain this repo but I recently massively expanded the scope of URLs I was using with it and it started to cause unforeseen issues when deployed to Cloudfront. I noticed that the compute utilization was getting into the 70-100 range, but more importantly I noticed some routes were not getting routed correctly. I believe this is because the routes in the generated function are exactly as they appear in the Next file tree.

This causes a problem is cases such as the following rewrites list:

  1. /posts/:postId/:commentId
  2. /posts/favorites/:postId
  3. /posts/favorites
  4. /posts

This list would be generated by an app folder with a posts folder with [:postId] and favorites inside of it, each of those with their own dynamic folder inside them. The issue is that a request to /posts/favorites/:postId would get caught by /posts/:postId/:commentId and cause an error in the browser since the IDs would be invalid.

To address both of these problems I took the rewrites list and turned it into a tree structure where the keys are the possible routes, destinations are marked with $_d and dynamic routes marked with $_p. The tree structure can be injected as a string into the Cloudfront template. I updated that to use the tree structure as well. I noticed the compute utilization in cloudfront is down to the single digits for me since making this change.

I copied the changes I made into a forked version of the repo and will leave them here as a PR. I won't claim they are fully tested, but can say this is what is working for my use case.

@zdenham
Copy link
Owner

zdenham commented Feb 20, 2025

Hi @lockehooper thanks for the PR.

This repo is not actively maintained, and personally I started using https://sst.dev/ rather than my own home-grown solution. That being said, if you bump the package.json version and test a bit I'd be happy to merge and hopefully someone finds it useful. Or can just leave the PR open indefinitely!

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.

2 participants