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

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

Merged
merged 4 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
55 changes: 48 additions & 7 deletions packages/adapters/fastify/web/src/web.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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',
Expand Down
41 changes: 29 additions & 12 deletions packages/adapters/fastify/web/src/web.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
Loading