NextJS with a root level catchall route for all URLs dynamic middleware that splits into components

Question

Here is the solution that I have. Due to the legacy product we have all of our URLs in one big Slug model in rails that maps URLs to models.

EG

[
 { 'Product 1' => '/product-1'},
 { 'Category 1' => '/category-1'}
}

Next.js it does not support sending a component from getStaticProps, so our [...slug].js winds up including every possible component (probably 50% of the total site code) leading to large code chunks where 80% of the code is not necessary for the page being rendered.

The solution was to use a custom middleware, but being the first time I've done something like this, I would like a review of my implementation. the pseudo code is:

• Find url lookup in cache
• If doesn't exist in cache, query API
• If url lookup successful, rewrite to the appropriate page
  • EG type Product => /p/${url}, type Category => /c/${url}
• If URL lookup unsuccessful, do nothing and let Next move on.

// middleware.js
import { NextResponse } from 'next/server'
import cache from 'memory-cache'
// Seachs an api to see if a particulary url slug has a specific type set.
// Then appends the path passed to next with a subfolder so that url can only
// have a specific component to render instead of loading them all in one [...slug].js
// or having to dynamically import them. It also uses a memory cache to not have to make 
// round trips to the api on every request.
export async function middleware(request) {
 const cacheTimeout = 3600 
 const knownBadPaths = ['_next']
 if (knownBadPaths.some(element => request.nextUrl.pathname.includes(element))) {
 let apiRes = cache.get(request.nextUrl.pathname)
 if (apiRes === null) {
 const nextSlug = request.nextUrl.pathname.substring(1).replaceAll('/', ',')
 const apiReq = await fetch(`${process.env.NEXT_PUBLIC_API_ROOT}/public/url_slugs/${nextSlug}`)
 apiRes = await apiReq.json()
 cache.put(request.nextUrl.pathname, apiRes, cacheTimeout)
 }
 if (apiRes.type === 'Category') {
 return NextResponse.rewrite(new URL(`/c${request.nextUrl.pathname}`, request.url))
 } else if (apiRes.type === 'Product') {
 return NextResponse.rewrite(new URL(`/p${request.nextUrl.pathname}`, request.url))
 } //... elseif for any other paths that need to be rewritten for specific types
 }
}
export const config = {
 matcher: '/:path*',
} 

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! In the case where apiRes = cache.get(request.nextUrl.pathname) is nullthen is it possible that the call to fetch() and apiRes = await apiReq.json() that apiRes might still be null or something that isn't an object?
• Romuloux: Thanks, I will look into it. Off the top of my head, the apiRes will never be null. The server might return a 404, 500, etc. That is OK for this use case as this router only cares about pages that exist and routing them to the correct component. It will check at most every hour with the api to see if the errors have been resolved as the cache should timeout.

Answer 1

Feedback

For a first attempt at a middleware it looks decent. It is a bit on the long side and hopefully the suggestions below can help with that. It is great that strict type comparisons are used. Another good thing is that it appears that const is used for any variable that does not need to be re-assigned.

Suggestions

Overwriting an existing variable

The variable apiRes is used not only for the value returned from cache.get() but also for the response from fetch(). That function returns a Promise which resolves to a Response object. Then apiRes is then assigned to the return value when apiRes.json() is called. The json() method returns a (regular, plain-old) JavaScript object. This could confuse someone reading the code - including your future self! Instead of re-assigning apiRes one could assign the value to a variable with a name like slug or slugData.

Separation of Concerns

The comment below suggests that there will likely be more lines added depending on the value of the type property:

} //... elseif for any other paths that need to be rewritten for specific types

As the function grows it can lead to code that is not well organized. It would be wise to create smaller functions to handle individual aspects and then have the main function call those small functions. Those functions likely do not need to be exported with the module. Perhaps one function could handle the lookup of the URL in the cache and adding it if it doesn't exist. Another function could handle the rewrite based on the type property. Then when adding code to handle another type one wouldn't have to look through the main function to find the place to modify but instead the function for handling the rewrite based on the type property.

Error handling

When calling functions that could throw an exception like fetch() it would be wise to catch exceptions and handle them. fetch() can throw various exceptions. One can wrap the call to fetch() in a try/catch block, or call .catch() after the call to fetch(). At minimum perhaps checking myRes.ok to see if it has a value of false would be wise.

Repeated nested property

There are many places where request.nextUrl.pathname is used. It could be stored in a variable with a shorter name so the lines that use it aren't so long. Destructuring assignment could be used to replace request with the two properties used - i.e. nextUrl and url.

Constant casing

Per the common convention of many popular style guides (e.g. Google JS, airBnB) the constant cacheTimeout could be named in UPPER_CASE - e.g. CACHE_TIMEOUT.

• Romuloux: Thank you. I will update my post with some refactored code. At this stage, it was more of a proof of concept. To see if I could dynamically route to a page component without a heavy performance hit. I still need to benchmark it.