From a36c5f95f949dd8675ee595883f77af25a158429 Mon Sep 17 00:00:00 2001 From: Tobbe Lundberg Date: Tue, 5 Mar 2024 10:35:21 +0100 Subject: [PATCH 1/4] fix(serve): Allow periods in most paths --- packages/adapters/fastify/web/src/web.ts | 37 +++++++++++++++++------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/packages/adapters/fastify/web/src/web.ts b/packages/adapters/fastify/web/src/web.ts index ac5e25b64f8e..4c629425e7d2 100644 --- a/packages/adapters/fastify/web/src/web.ts +++ b/packages/adapters/fastify/web/src/web.ts @@ -86,17 +86,34 @@ export async function redwoodFastifyWeb( 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) + // 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 (requestedExtension && 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) }) } From 70f2cce15fbcfbf60e19e03de29033ddc1b42399 Mon Sep 17 00:00:00 2001 From: Tobbe Lundberg Date: Tue, 5 Mar 2024 10:42:47 +0100 Subject: [PATCH 2/4] CHANGELOG.md --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a20a32767ec..94df6dbedd01 100644 --- a/CHANGELOG.md +++ b/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(deps): update prisma monorepo to v5.10.2 (#10088) This release updates Prisma to v5.10.2. Here are quick links to all the release notes since the last version (v5.9.1): From ff876f705ff185c4e92972d5c4098d564baa5ef6 Mon Sep 17 00:00:00 2001 From: Tobbe Lundberg Date: Tue, 5 Mar 2024 10:44:08 +0100 Subject: [PATCH 3/4] Rename variable --- packages/adapters/fastify/web/src/web.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/adapters/fastify/web/src/web.ts b/packages/adapters/fastify/web/src/web.ts index 4c629425e7d2..d5268d58a5dd 100644 --- a/packages/adapters/fastify/web/src/web.ts +++ b/packages/adapters/fastify/web/src/web.ts @@ -84,7 +84,7 @@ 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 ?? '') + 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 @@ -100,7 +100,7 @@ export async function redwoodFastifyWeb( // See the discussions in https://github.com/redwoodjs/redwood/pull/9272 // and https://github.com/redwoodjs/redwood/issues/9969 - if (requestedExtension && urlData.path?.startsWith('/assets/')) { + 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 // From 75ec46e78cec2425a4a8d98fd63d1210fb8f4168 Mon Sep 17 00:00:00 2001 From: Tobbe Lundberg Date: Tue, 5 Mar 2024 13:24:12 +0100 Subject: [PATCH 4/4] Update/add tests --- packages/adapters/fastify/web/src/web.test.ts | 55 ++++++++++++++++--- 1 file changed, 48 insertions(+), 7 deletions(-) diff --git a/packages/adapters/fastify/web/src/web.test.ts b/packages/adapters/fastify/web/src/web.test.ts index 7e49bb4720f8..0f3af359396d 100644 --- a/packages/adapters/fastify/web/src/web.test.ts +++ b/packages/adapters/fastify/web/src/web.test.ts @@ -1,6 +1,7 @@ -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' @@ -8,7 +9,7 @@ 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',