-
-
Notifications
You must be signed in to change notification settings - Fork 923
feat(ssg): add redirect plugin #4599
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
base: main
Are you sure you want to change the base?
Changes from all commits
898e760
5b1dd0c
1c3cc1a
00137c9
7b4556b
dd4a467
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,3 +11,4 @@ export { | |
| disableSSG, | ||
| onlySSG, | ||
| } from './middleware' | ||
| export { redirectPlugin } from './plugins' | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default plugin is an object, but the redirect plugin is a function. Should we standardize it to functions for consistency?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, I think |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import { html } from '../html' | ||
| import type { SSGPlugin } from './ssg' | ||
|
|
||
| const generateRedirectHtml = (from: string, to: string) => { | ||
| // prettier-ignore | ||
| const content = html`<!DOCTYPE html> | ||
| <title>Redirecting to: ${to}</title> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a rare case if passing the script into import { html } from '../html'And you can esapce
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I refactored the code to use the html helper and added test cases related to escaping. Double escaping might not be necessary. Also, it could be useful if the html helper had an option to get a minified string. If you cannot think of many use cases for it, it may not be needed. |
||
| <meta http-equiv="refresh" content="0;url=${to}" /> | ||
| <meta name="robots" content="noindex" /> | ||
| <link rel="canonical" href="${to}" /> | ||
| <body> | ||
| <a href="${to}">Redirecting from <code>${from}</code> to <code>${to}</code></a> | ||
| </body> | ||
| ` | ||
| return content.toString().replace(/\n/g, '') | ||
| } | ||
|
|
||
| /** | ||
| * Redirect plugin for Hono SSG. | ||
| * | ||
| * Generates HTML redirect pages for HTTP 301 and 302 responses. | ||
| * When used with `defaultPlugin`, place `redirectPlugin` before it, because `defaultPlugin` skips non-200 responses. | ||
| * | ||
| * @returns A SSGPlugin that generates HTML redirect pages. | ||
| * | ||
| * @experimental | ||
| * `redirectPlugin` is an experimental feature. | ||
| * The API might be changed. | ||
| */ | ||
| export const redirectPlugin = (): SSGPlugin => { | ||
| return { | ||
| afterResponseHook: (res) => { | ||
| if (res.status === 301 || res.status === 302) { | ||
| const location = res.headers.get('Location') | ||
| if (!location) { | ||
| return false | ||
| } | ||
| const html = generateRedirectHtml('', location) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't specify the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to implement it. The I also thought of another option, but this approach might break when running in parallel. (For production use, we would need to use a FIFO queue, but in this PoC I implemented it in a straightforward way.) export const redirectPlugin = (): SSGPlugin => {
const requestedUrls: string[] = []
return {
beforeRequestHook: (req) => {
requestedUrls.push(new URL(req.url).pathname)
return req
},
afterResponseHook: (res) => {
if (res.status === 301 || res.status === 302) {
const location = res.headers.get('Location')
if (!location) {
return false
}
const from = requestedUrls.shift()
if (!from) {
return false
}
const body = generateRedirectHtml(from, location)
return new Response(body, {
status: 200,
headers: { 'Content-Type': 'text/html; charset=utf-8' },
})
}
return res
},
}
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about removing At the very least, the user knows where it comes from because they access the URL directly. And I think the information about |
||
| return new Response(html, { | ||
| status: 200, | ||
| headers: { 'Content-Type': 'text/html; charset=utf-8' }, | ||
| }) | ||
| } | ||
| return res | ||
| }, | ||
| } | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Build-in plugins test should be separated to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |
| onlySSG, | ||
| ssgParams, | ||
| } from './middleware' | ||
| import { redirectPlugin } from './plugins' | ||
| import { | ||
| defaultExtensionMap, | ||
| fetchRoutesContent, | ||
|
|
@@ -254,6 +255,79 @@ describe('toSSG function', () => { | |
| ) | ||
| }) | ||
|
|
||
| it('should generate redirect HTML for 301/302 route responses using plugin', async () => { | ||
| const writtenFiles: Record<string, string> = {} | ||
| const fsMock: FileSystemModule = { | ||
| writeFile: (path, data) => { | ||
| writtenFiles[path] = typeof data === 'string' ? data : data.toString() | ||
| return Promise.resolve() | ||
| }, | ||
| mkdir: vi.fn(() => Promise.resolve()), | ||
| } | ||
| const app = new Hono() | ||
| app.get('/old', (c) => c.redirect('/new')) | ||
| app.get('/new', (c) => c.html('New Page')) | ||
|
|
||
| await toSSG(app, fsMock, { dir: './static', plugins: [redirectPlugin()] }) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of a situation where you use it with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think updating the documentation is essential. Additionally, it might be helpful to add
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added TSDoc and tests for plugin ordering.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. Let's discuss another place. |
||
|
|
||
| expect(writtenFiles['static/old.html']).toBeDefined() | ||
| const content = writtenFiles['static/old.html'] | ||
| // Should contain meta refresh | ||
| expect(content).toContain('meta http-equiv="refresh" content="0;url=/new"') | ||
| // Should contain canonical | ||
| expect(content).toContain('rel="canonical" href="/new"') | ||
| // Should contain robots noindex | ||
| expect(content).toContain('<meta name="robots" content="noindex" />') | ||
| // Should contain link anchor | ||
| expect(content).toContain('<a href="/new">Redirecting from') | ||
| // Should contain a body element that includes the anchor | ||
| expect(content).toMatch(/<body[^>]*>[\s\S]*<a href=\"\/new\">[\s\S]*<\/body>/) | ||
| }) | ||
|
|
||
| it('should escape Location header values when generating redirect HTML', async () => { | ||
| const writtenFiles: Record<string, string> = {} | ||
| const fsMock: FileSystemModule = { | ||
| writeFile: (path, data) => { | ||
| writtenFiles[path] = typeof data === 'string' ? data : data.toString() | ||
| return Promise.resolve() | ||
| }, | ||
| mkdir: vi.fn(() => Promise.resolve()), | ||
| } | ||
|
|
||
| const maliciousLocation = '/new"> <script>alert(1)</script>' | ||
| const app = new Hono() | ||
| app.get( | ||
| '/evil', | ||
| (c) => new Response(null, { status: 301, headers: { Location: maliciousLocation } }) | ||
| ) | ||
|
|
||
| await toSSG(app, fsMock, { dir: './static', plugins: [redirectPlugin()] }) | ||
|
|
||
| const content = writtenFiles['static/evil.html'] | ||
| expect(content).toBeDefined() | ||
| expect(content).not.toContain('<script>alert(1)</script>') | ||
| expect(content).toContain('<script>alert(1)</script>') | ||
| expect(content).toContain('"') | ||
| }) | ||
|
|
||
| it('should skip generating a redirect HTML when 301/302 has no Location header', async () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The specification mandates that redirect responses must include a Footnotes |
||
| const writtenFiles: Record<string, string> = {} | ||
| const fsMock: FileSystemModule = { | ||
| writeFile: (path, data) => { | ||
| writtenFiles[path] = typeof data === 'string' ? data : data.toString() | ||
| return Promise.resolve() | ||
| }, | ||
| mkdir: vi.fn(() => Promise.resolve()), | ||
| } | ||
| const app = new Hono() | ||
| // Return a 301 without Location header | ||
| app.get('/bad', (c) => new Response(null, { status: 301 })) | ||
|
|
||
| await toSSG(app, fsMock, { dir: './static', plugins: [redirectPlugin()] }) | ||
|
|
||
| expect(writtenFiles['static/bad.html']).toBeUndefined() | ||
| }) | ||
|
|
||
| it('should handle asynchronous beforeRequestHook correctly', async () => { | ||
| const beforeRequestHook: BeforeRequestHook = async (req) => { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
|
|
@@ -906,6 +980,42 @@ describe('SSG Plugin System', () => { | |
| expect(fsMock.writeFile).toHaveBeenCalledWith('static/blog.html', '<h1>Blog - Modified</h1>') | ||
| }) | ||
|
|
||
| it('redirectPlugin before defaultPlugin generates redirect HTML', async () => { | ||
| const writtenFiles: Record<string, string> = {} | ||
| const fsMockLocal: FileSystemModule = { | ||
| writeFile: (path, data) => { | ||
| writtenFiles[path] = typeof data === 'string' ? data : data.toString() | ||
| return Promise.resolve() | ||
| }, | ||
| mkdir: vi.fn(() => Promise.resolve()), | ||
| } | ||
|
|
||
| const app = new Hono() | ||
| app.get('/old', (c) => c.redirect('/new')) | ||
| app.get('/new', (c) => c.html('New Page')) | ||
|
|
||
| await toSSG(app, fsMockLocal, { dir: './static', plugins: [redirectPlugin(), defaultPlugin] }) | ||
| expect(writtenFiles['static/old.html']).toBeDefined() | ||
| }) | ||
|
|
||
| it('redirectPlugin after defaultPlugin does not generate redirect HTML', async () => { | ||
| const writtenFiles: Record<string, string> = {} | ||
| const fsMockLocal: FileSystemModule = { | ||
| writeFile: (path, data) => { | ||
| writtenFiles[path] = typeof data === 'string' ? data : data.toString() | ||
| return Promise.resolve() | ||
| }, | ||
| mkdir: vi.fn(() => Promise.resolve()), | ||
| } | ||
|
|
||
| const app = new Hono() | ||
| app.get('/old', (c) => c.redirect('/new')) | ||
| app.get('/new', (c) => c.html('New Page')) | ||
|
|
||
| await toSSG(app, fsMockLocal, { dir: './static', plugins: [defaultPlugin, redirectPlugin()] }) | ||
| expect(writtenFiles['static/old.html']).toBeUndefined() | ||
| }) | ||
|
|
||
| it('should correctly apply plugins with afterGenerateHook', async () => { | ||
| const additionalFiles = ['sitemap.xml', 'robots.txt'] | ||
| const plugin: SSGPlugin = { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since built-in plugins should only handle lightweight ones, I decided to manage multiple plugins in a single
plugins.tsfile. This should make updating exports slightly more convenient.