fix(serve): Allow periods in most paths (#10114)

CHANGELOG.md

@@ -2,6 +2,19 @@
 
 ## Unreleased
 
+- fix(serve): Allow periods in most paths (#10114)
+
+  Partial fix for route paths with periods in them.
+
+  It's only "partial" because it doesn't fix it for `yarn rw dev`, as that's a
+  Vite bug
+  ([vitejs/vite#2415 (comment)](https://github.com/vitejs/vite/issues/2415#issuecomment-1720814355)).
+  And there's also an edge case for yarn rw serve where this doesn't fully
+  handle client-side routes that start with /assets/ and that also have a
+  last-segment that accepts a period, like /assets/client-route-image.jpg
+
+  Fixes #9969
+
 - fix(esm): fix initial ESM blockers for Redwood apps (#10083) by @jtoar and @Josh-Walker-GM
 
   This PR makes some initial fixes that were required for making a Redwood app ESM. Redwood apps aren't ready to transition to ESM yet, but we're working towards it and these changes were backwards-compatible.

packages/adapters/fastify/web/src/web.test.ts

@@ -1,14 +1,15 @@
-import fs from 'fs'
-import path from 'path'
+import * as fs from 'fs'
+import * as path from 'path'
 
+import type { FastifyInstance } from 'fastify'
 import Fastify from 'fastify'
 import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
 
 import { getPaths } from '@redwoodjs/project-config'
 
 import { redwoodFastifyWeb } from './web'
 
-let original_RWJS_CWD
+let original_RWJS_CWD: string
 
 beforeAll(() => {
   original_RWJS_CWD = process.env.RWJS_CWD
@@ -24,7 +25,7 @@ describe('redwoodFastifyWeb', () => {
   console.log = vi.fn()
 
   // Set up and teardown the fastify instance with options.
-  let fastifyInstance
+  let fastifyInstance: FastifyInstance
 
   const port = 8910
 
@@ -248,16 +249,56 @@ describe('redwoodFastifyWeb', () => {
   })
 
   describe("returns a 404 for assets that can't be found", () => {
-    it("returns a 404 for non-html assets that can't be found", async () => {
+    it("returns a 404 for assets that can't be found", async () => {
       const res = await fastifyInstance.inject({
         method: 'GET',
-        url: '/kittens.png',
+        url: '/assets/kittens.png',
       })
 
       expect(res.statusCode).toBe(404)
     })
 
-    it('handles "."s in routes', async () => {
+    // This is testing current behavior - not ideal behavior. Feel free to
+    // update this test if you change the behavior.
+    // It's for the (hopefully rare) case where someone has a client-side
+    // route for /assets
+    it('returns a 200 for plain files, even in /assets/', async () => {
+      const res = await fastifyInstance.inject({
+        method: 'GET',
+        url: '/assets/kittens',
+      })
+
+      expect(res.statusCode).toBe(200)
+    })
+
+    it('handles "."s in route segments', async () => {
+      const res = await fastifyInstance.inject({
+        method: 'GET',
+        url: '/my.page/foo',
+      })
+
+      expect(res.statusCode).toBe(200)
+    })
+
+    it('handles "."s in last route segment', async () => {
+      const res = await fastifyInstance.inject({
+        method: 'GET',
+        url: '/foo/my.page',
+      })
+
+      expect(res.statusCode).toBe(200)
+    })
+
+    it('handles filenames in route segments', async () => {
+      const res = await fastifyInstance.inject({
+        method: 'GET',
+        url: '/file-route/fake.js',
+      })
+
+      expect(res.statusCode).toBe(200)
+    })
+
+    it('handles "."s in query params', async () => {
       const res = await fastifyInstance.inject({
         method: 'GET',
         url: '/my-page?loading=spinner.blue',

packages/adapters/fastify/web/src/web.ts

@@ -84,19 +84,36 @@ export async function redwoodFastifyWeb(
   // For SPA routing, fallback on unmatched routes and let client-side routing take over
   fastify.setNotFoundHandler({}, (req, reply) => {
     const urlData = req.urlData()
-    const requestedExtension = path.extname(urlData.path ?? '')
-
-    // Paths with no extension (`/about`) or an .html extension (`/about.html`)
-    // should be handled by the client side router.
-    // See the discussion in https://github.com/redwoodjs/redwood/pull/9272.
-    if (requestedExtension === '' || requestedExtension === '.html') {
-      reply.header('Content-Type', 'text/html; charset=UTF-8')
-      return reply.sendFile(fallbackIndexPath)
+    const requestHasExtension = !!path.extname(urlData.path ?? '')
+
+    // Further up in this file we use `fastifyStatic` to serve files from the
+    // /web/dist folder. Most often for files like AboutPage-12ab34cd.js or
+    // some css file.
+    // Requests for other paths should most often be handled by client side
+    // routing. Like requests /about or /about.html.
+    // One exception for this is requests for assets that don't exist anymore.
+    // Like AboutPage-old_hash.js. These requests should return 404.
+    // The problem is we don't know what those assets are. So the best we can
+    // do is to return 404 for all requests for files in /assets that have an
+    // extension.
+    //
+    // See the discussions in https://github.com/redwoodjs/redwood/pull/9272
+    // and https://github.com/redwoodjs/redwood/issues/9969
+
+    if (requestHasExtension && urlData.path?.startsWith('/assets/')) {
+      // If we got here, the user is most likely requesting an asset with an
+      // extension (like `assets/AboutPage-xyz789.js`) that doesn't exist
+      //
+      // NOTE: This is a best guess, and could be wrong. The user could have
+      // a client-side route setup for /assets/client-side/{...} and in that
+      // case we really should pass this on to the client-side router instead
+      // of returning 404.
+      reply.code(404)
+      return reply.send('Not Found')
     }
 
-    // If we got here, the user is requesting an asset with an extension
-    // (like `profile.png`) that doesn't exist
-    reply.code(404)
-    return reply.send('Not Found')
+    // Let client-side routing take over
+    reply.header('Content-Type', 'text/html; charset=UTF-8')
+    return reply.sendFile(fallbackIndexPath)
   })
 }