Merge pull request #293 from gadget-inc/dont-render-on-redirect

Ensure we don't render if a hook or the render function already sends a reply

Author: airhorns

packages/fastify-renderer/src/node/index.ts

@@ -88,19 +88,21 @@ const FastifyRenderer = fp<FastifyRendererOptions>(
         routeOptions.handler = wrap('fastify-renderer.handler', async function (this: FastifyInstance, request, reply) {
           const props = await oldHandler.call(this, request, reply)
 
-          void reply.header('Vary', 'Accept')
-          switch (request.accepts().type(['html', 'json'])) {
-            case 'json':
-              await reply.type('application/json').send({ props })
-              break
-            case 'html':
-              void reply.type('text/html')
-              const render: Render<typeof props> = { ...renderableRoute, request, reply, props, renderable }
-              await plugin.renderer.render(render)
-              break
-            default:
-              await reply.type('text/plain').send('Content type not supported')
-              break
+          if (!reply.sent) {
+            void reply.header('Vary', 'Accept')
+            switch (request.accepts().type(['html', 'json'])) {
+              case 'json':
+                await reply.type('application/json').send({ props })
+                break
+              case 'html':
+                void reply.type('text/html')
+                const render: Render<typeof props> = { ...renderableRoute, request, reply, props, renderable }
+                await plugin.renderer.render(render)
+                break
+              default:
+                await reply.type('text/plain').send('Content type not supported')
+                break
+            }
           }
         })
       }

packages/fastify-renderer/test/FastifyRenderer.spec.ts

@@ -3,132 +3,93 @@ import path from 'path'
 import * as Vite from 'vite'
 import FastifyRenderer, { build } from '../src/node'
 import { FastifyRendererPlugin } from '../src/node/Plugin'
-import { ReactRenderer } from '../src/node/renderers/react/ReactRenderer'
 import { kRenderOptions } from '../src/node/symbols'
 import { newFastify } from './helpers'
 
+const testComponent = require.resolve(path.join(__dirname, 'fixtures', 'test-module.tsx'))
+const testLayoutComponent = require.resolve(path.join(__dirname, 'fixtures', 'test-layout.tsx'))
+const options = { vite: { root: __dirname, logLevel: (process.env.LOG_LEVEL ?? 'info') as any } }
+
 describe('FastifyRenderer', () => {
-  beforeEach(() => {
+  let server
+  beforeEach(async () => {
     jest.clearAllMocks()
+
+    server = await newFastify()
+    await server.register(FastifyRenderer, options)
+    server.setRenderConfig({
+      base: '',
+      layout: testLayoutComponent,
+    })
   })
 
   test('should throw if the fastify-renderer plugin was already registered in the same fastify context', async () => {
-    const server = await newFastify()
+    server = await newFastify()
 
-    await server.register(FastifyRenderer)
+    await server.register(FastifyRenderer, options)
     await expect(server.register(FastifyRenderer)).rejects.toThrow("The decorator 'vite' has already been added!")
   })
 
   test('should throw if the fastify-renderer plugin was already registered in a different context', async () => {
-    const server = await newFastify()
+    server = await newFastify()
 
-    await server.register(FastifyRenderer)
+    await server.register(FastifyRenderer, options)
     await expect(server.register(async (instance) => instance.register(FastifyRenderer))).rejects.toThrow()
   })
 
   test('should decorate the fastify instance with a "setRenderConfig" method', async () => {
-    const server = await newFastify()
-    await server.register(FastifyRenderer)
+    server = await newFastify()
+    await server.register(FastifyRenderer, options)
     expect(server.setRenderConfig).toBeInstanceOf(Function)
+    server.setRenderConfig({
+      base: '/foo',
+    })
   })
 
   test('should set default render config for the instance', async () => {
-    const server = await newFastify()
-
-    expect(server[kRenderOptions]).toBeUndefined()
-    await server.register(FastifyRenderer)
     expect(server[kRenderOptions]).toBeInstanceOf(Object)
   })
 
   test('should set the renderOptions on the new fastify instance context', async () => {
-    const server = await newFastify()
-
-    await server.register(FastifyRenderer)
     await server.register(async (instance) => {
       expect(server[kRenderOptions]).toEqual(instance[kRenderOptions])
     })
   })
 
   test('should mount vite routes at a prefix to avoid collision with user routes', async () => {
-    const server = await newFastify()
-
-    await server.register(FastifyRenderer)
     expect(server.printRoutes()).toMatch('/.vite/')
   })
 
-  // test.skip('should use config from vite dev server in dev mode', async () => {
-  // });
-
   test('should close vite devServer when fastify server is closing in dev mode', async () => {
     const devServer = await Vite.createServer()
     const closeSpy = jest.spyOn(devServer, 'close')
     jest.spyOn(Vite, 'createServer').mockImplementation(async () => devServer)
 
-    const server = await newFastify()
-    await server.register(FastifyRenderer, { devMode: true })
+    server = await newFastify()
+    await server.register(FastifyRenderer, { ...options, devMode: true })
     await server.listen(0)
     await server.close()
 
     expect(closeSpy).toHaveBeenCalled()
   })
 
   test('should do nothing if the registered route is not renderable', async () => {
-    const server = await newFastify()
     const registerRouteSpy = jest.spyOn(FastifyRendererPlugin.prototype, 'registerRoute')
 
-    await server.register(FastifyRenderer)
     server.get('/', async (_request, reply) => reply.send('Hello'))
     await server.inject({ method: 'GET', url: '/' })
 
     expect(registerRouteSpy).toHaveBeenCalledTimes(0)
   })
 
   test("should register the route in the plugin if it's renderable", async () => {
-    const server = await newFastify()
     const registerRouteSpy = jest.spyOn(FastifyRendererPlugin.prototype, 'registerRoute').mockImplementation(jest.fn())
 
-    await server.register(FastifyRenderer)
-    server.get('/', { render: 'renderable-module-path' }, async (request, reply) => reply.send('Hello'))
+    server.get('/', { render: testComponent }, async (request, reply) => reply.send('Hello'))
     await server.inject({ method: 'GET', url: '/' })
 
     expect(registerRouteSpy).toHaveBeenCalledTimes(1)
   })
-
-  test('should return the route props if content-type is application/json', async () => {
-    const server = await newFastify()
-    jest.spyOn(FastifyRendererPlugin.prototype, 'registerRoute').mockImplementation(jest.fn())
-
-    await server.register(FastifyRenderer)
-    server.get('/', { render: 'renderable-module-path' }, async (_request, _reply) => ({ a: 1, b: 2 }))
-    const response = await server.inject({ method: 'GET', url: '/', headers: { Accept: 'application/json' } })
-
-    expect(response.body).toEqual(JSON.stringify({ props: { a: 1, b: 2 } }))
-  })
-
-  test('should render and return html if content-type is text/html', async () => {
-    const htmlContent = '<html><body>test content</body></html>'
-
-    jest.spyOn(ReactRenderer.prototype, 'render').mockImplementation(async (render) => render.reply.send(htmlContent))
-    jest.spyOn(FastifyRendererPlugin.prototype, 'registerRoute').mockImplementation(jest.fn())
-
-    const server = await newFastify()
-    await server.register(FastifyRenderer)
-    server.get('/', { render: 'renderable-module-path' }, async (_request, _reply) => ({ a: 1, b: 2 }))
-    const response = await server.inject({ method: 'GET', url: '/', headers: { Accept: 'text/html' } })
-
-    expect(response.body).toEqual(htmlContent)
-  })
-
-  test('should set content-type to text/plain and return a message', async () => {
-    const server = await newFastify()
-    jest.spyOn(FastifyRendererPlugin.prototype, 'registerRoute').mockImplementation(jest.fn())
-
-    await server.register(FastifyRenderer)
-    server.get('/', { render: 'renderable-module-path' }, async (_request, _reply) => ({ a: 1, b: 2 }))
-    const response = await server.inject({ method: 'GET', url: '/', headers: { Accept: 'other' } })
-
-    expect(response.body).toEqual('Content type not supported')
-  })
 })
 
 describe('build()', () => {
@@ -139,7 +100,7 @@ describe('build()', () => {
 
   test('should build client and server side assets', async () => {
     const server = await newFastify()
-    await server.register(FastifyRenderer)
+    await server.register(FastifyRenderer, options)
     await server.listen(0)
 
     jest.spyOn(fs, 'writeFile').mockImplementation(jest.fn())

packages/fastify-renderer/test/fixtures/test-layout.tsx

@@ -0,0 +1,7 @@
+import React, { Suspense } from 'react'
+import { LayoutProps } from '../../src/client/react'
+
+// eslint-disable-next-line react/display-name
+export default function (props: LayoutProps) {
+  return <Suspense fallback={'Loading'}>{props.children}</Suspense>
+}

packages/fastify-renderer/test/fixtures/test-module.tsx

@@ -0,0 +1,14 @@
+import React from 'react'
+
+// eslint-disable-next-line react/display-name
+export default function (props: { a: string; b: number }) {
+  if (typeof props.a == 'undefined') {
+    throw new Error('expected to be passed props from render function')
+  }
+  return (
+    <>
+      <h1>{props.a}</h1>
+      <p>{props.b}</p>
+    </>
+  )
+}

packages/fastify-renderer/test/helpers.ts

@@ -5,8 +5,10 @@ import { FastifyRendererOptions, FastifyRendererPlugin } from '../src/node/Plugi
 import { RenderBus } from '../src/node/RenderBus'
 import { ReactRenderer, ReactRendererOptions } from '../src/node/renderers/react/ReactRenderer'
 
+const logLevel = process.env.LOG_LEVEL || 'error'
+
 export const newFastify = async (options?: FastifyServerOptions) => {
-  const server = fastify(options)
+  const server = fastify({ ...options, logger: { level: logLevel, prettyPrint: true } })
   await server.register(fastifyAccepts)
   await server.register(Middie)
   return server

packages/fastify-renderer/test/routes.spec.ts

@@ -0,0 +1,76 @@
+import path from 'path'
+import FastifyRenderer from '../src/node'
+import { newFastify } from './helpers'
+
+const testComponent = require.resolve(path.join(__dirname, 'fixtures', 'test-module.tsx'))
+const testLayoutComponent = require.resolve(path.join(__dirname, 'fixtures', 'test-layout.tsx'))
+const options = { vite: { root: __dirname, logLevel: (process.env.LOG_LEVEL ?? 'info') as any } }
+
+describe('FastifyRenderer', () => {
+  let server
+  beforeAll(async () => {
+    server = await newFastify()
+    await server.register(FastifyRenderer, options)
+    server.setRenderConfig({
+      base: '',
+      layout: testLayoutComponent,
+    })
+
+    server.get('/plain', async (_request, reply) => reply.send('Hello'))
+    server.get('/render-test', { render: testComponent }, async (_request, _reply) => ({ a: 1, b: 2 }))
+    server.get(
+      '/early-hook-reply',
+      {
+        preValidation: async (_request, reply) => {
+          await reply.code(201).send('hello')
+        },
+        render: testComponent,
+      },
+      async (_request, _reply) => ({ a: 1, b: 2 })
+    )
+    server.get('/early-handler-reply', { render: testComponent }, async (request, reply) => {
+      await reply.code(201).send('hello')
+      return { a: 1, b: 2 }
+    })
+    await server.ready()
+  })
+
+  test('should return the route props if content-type is application/json', async () => {
+    const response = await server.inject({
+      method: 'GET',
+      url: '/render-test',
+      headers: { Accept: 'application/json' },
+    })
+
+    expect(response.body).toEqual(JSON.stringify({ props: { a: 1, b: 2 } }))
+  })
+
+  test('should render and return html if content-type is text/html', async () => {
+    const response = await server.inject({ method: 'GET', url: '/render-test', headers: { Accept: 'text/html' } })
+
+    expect(response.body).toMatch('<h1>1</h1><p>2</p>')
+  })
+  test('for unknown content types should set content-type to text/plain and return a message', async () => {
+    const response = await server.inject({ method: 'GET', url: '/render-test', headers: { Accept: 'other' } })
+
+    expect(response.body).toEqual('Content type not supported')
+  })
+
+  test('should not render anything if a hook replies before the render phase is reached', async () => {
+    const response = await server.inject({ method: 'GET', url: '/early-hook-reply', headers: { Accept: 'text/html' } })
+
+    expect(response.statusCode).toEqual(201)
+    expect(response.body).toEqual('hello')
+  })
+
+  test('should not render anything if the handler replies before its end', async () => {
+    const response = await server.inject({
+      method: 'GET',
+      url: '/early-handler-reply',
+      headers: { Accept: 'text/html' },
+    })
+
+    expect(response.statusCode).toEqual(201)
+    expect(response.body).toEqual('hello')
+  })
+})

yarn.lock

@@ -1356,21 +1356,14 @@
   resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.7.3.tgz#2ab0d5da2e5815f94b0b9d4b95d1e5f243ab2ca7"
   integrity sha512-KfRL3PuHmqQLOG+2tGpRO26Ctg+Cq1E01D2DMriKEATHgWLfeNDmq9e29Q9WIky0dQ3NPkd1mzYH8Lm936Z9qw==
 
-"@types/[email protected]":
+"@types/[email protected]", "@types/react-dom@^17.0.11":
   version "17.0.4"
   resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-17.0.4.tgz#d65159a847aca2a0fc87a7544a2f8fece8754d04"
   integrity sha512-Wb6rlnPJfqbhpkvYN39y1NM/pOGGPzzIRquu0RdUMvTwgXNvASFO7pdtrtvyxGTQNb9wzBaQxXAWDdEqegZw2A==
   dependencies:
     "@types/react" "*"
 
-"@types/react-dom@^17.0.11":
-  version "17.0.11"
-  resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-17.0.11.tgz#e1eadc3c5e86bdb5f7684e00274ae228e7bcc466"
-  integrity sha512-f96K3k+24RaLGVu/Y2Ng3e1EbZ8/cVJvypZWd7cy0ofCBaf2lcM46xNhycMZ2xGwbBjRql7hOlZ+e2WlJ5MH3Q==
-  dependencies:
-    "@types/react" "*"
-
-"@types/react@*", "@types/[email protected]":
+"@types/react@*", "@types/[email protected]", "@types/react@^17.0.36":
   version "17.0.4"
   resolved "https://registry.yarnpkg.com/@types/react/-/react-17.0.4.tgz#a67c6f7a460d2660e950d9ccc1c2f18525c28220"
   integrity sha512-onz2BqScSFMoTRdJUZUDD/7xrusM8hBA2Fktk2qgaTYPCgPvWnDEgkrOs8hhPUf2jfcIXkJ5yK6VfYormJS3Jw==
@@ -1379,15 +1372,6 @@
     "@types/scheduler" "*"
     csstype "^3.0.2"
 
-"@types/react@^17.0.36":
-  version "17.0.38"
-  resolved "https://registry.yarnpkg.com/@types/react/-/react-17.0.38.tgz#f24249fefd89357d5fa71f739a686b8d7c7202bd"
-  integrity sha512-SI92X1IA+FMnP3qM5m4QReluXzhcmovhZnLNm3pyeQlooi02qI7sLiepEYqT678uNiyc25XfCqxREFpy3W7YhQ==
-  dependencies:
-    "@types/prop-types" "*"
-    "@types/scheduler" "*"
-    csstype "^3.0.2"
-
 "@types/sanitize-filename@^1.6.3":
   version "1.6.3"
   resolved "https://registry.yarnpkg.com/@types/sanitize-filename/-/sanitize-filename-1.6.3.tgz#182ebd5658fbd3fe36bcb771daad8b2623371705"
@@ -5967,14 +5951,6 @@ [email protected]:
     loose-envify "^1.1.0"
     object-assign "^4.1.1"
 
-scheduler@^0.20.2:
-  version "0.20.2"
-  resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.20.2.tgz#4baee39436e34aa93b4874bddcbf0fe8b8b50e91"
-  integrity sha512-2eWfGgAqqWFGqtdMmcL5zCMK1U8KlXv8SQFGglL3CEtd0aDVDWgeF/YoCmvln55m5zSk3J/20hTaSBeSObsQDQ==
-  dependencies:
-    loose-envify "^1.1.0"
-    object-assign "^4.1.1"
-
 secure-json-parse@^2.0.0:
   version "2.4.0"
   resolved "https://registry.yarnpkg.com/secure-json-parse/-/secure-json-parse-2.4.0.tgz#5aaeaaef85c7a417f76271a4f5b0cc3315ddca85"