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

feat(ssg): Ignore Dynamic Route #1990

Merged
merged 7 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 29 additions & 5 deletions deno_dist/helper/ssg/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,41 @@ export const fetchRoutesContent = async <
const baseURL = 'http://localhost'

for (const route of inspectRoutes(app)) {
if (route.isMiddleware) continue
if (route.isMiddleware || isDynamicRoute(route.path)) continue

const url = new URL(route.path, baseURL).toString()
const contentData = await fetchRouteContent(app, route.path, baseURL)
if (contentData) {
htmlMap.set(route.path, contentData)
}
}

return htmlMap
}

interface RouteContent {
content: string | ArrayBuffer
mimeType: string
}

const fetchRouteContent = async <E extends Env = Env, S extends Schema = {}>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you include these fetchRouteContent things in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't need it, so I reverted it.

app: Hono<E, S>,
path: string,
baseURL: string
): Promise<RouteContent | null> => {
try {
const url = new URL(path, baseURL).toString()
const response = await app.fetch(new Request(url))
const mimeType = response.headers.get('Content-Type')?.split(';')[0] || 'text/plain'
const content = await parseResponseContent(response)

htmlMap.set(route.path, { content, mimeType })
return { content, mimeType }
} catch (error) {
console.error(`Error fetching content for path: ${path}`, error)
return null
}
}

return htmlMap
const isDynamicRoute = (path: string): boolean => {
return /:\w+|\*/.test(path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The determination of the named path parameters here is incorrect.

/foo:bar is not a dynamic route. You can see the spec here:

const match = label.match(/^\:([^\{\}]+)(?:\{(.+)\})?$/)

So you have to split the path with / and determine if there is a : at the beginning of it or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

}

/**
Expand Down
28 changes: 27 additions & 1 deletion src/helper/ssg/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ describe('fetchRoutesContent function', () => {

it('should handle errors correctly', async () => {
vi.spyOn(app, 'fetch').mockRejectedValue(new Error('Network error'))
await expect(fetchRoutesContent(app)).rejects.toThrow('Network error')
const result = await fetchRoutesContent(app)
expect(result).toBeInstanceOf(Map)
vi.restoreAllMocks()
})
})
Expand Down Expand Up @@ -150,3 +151,28 @@ describe('saveContentToFiles function', () => {
})

})

describe('Dynamic route handling', () => {
let app: Hono
beforeEach(() => {
app = new Hono()
app.get('/shops/:id', (c) => c.html('Shop Page'))
app.get('/shops/:id/:comments([0-9]+)', (c) => c.html('Comments Page'))
app.get('/foo/*', (c) => c.html('Foo Page'))
})

it('should skip /shops/:id dynamic route', async () => {
const htmlMap = await fetchRoutesContent(app)
expect(htmlMap.has('/shops/:id')).toBeFalsy()
})

it('should skip /shops/:id/:comments([0-9]+) dynamic route', async () => {
const htmlMap = await fetchRoutesContent(app)
expect(htmlMap.has('/shops/:id/:comments([0-9]+)')).toBeFalsy()
})

it('should skip /foo/* dynamic route', async () => {
const htmlMap = await fetchRoutesContent(app)
expect(htmlMap.has('/foo/*')).toBeFalsy()
})
})
34 changes: 29 additions & 5 deletions src/helper/ssg/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,41 @@ export const fetchRoutesContent = async <
const baseURL = 'http://localhost'

for (const route of inspectRoutes(app)) {
if (route.isMiddleware) continue
if (route.isMiddleware || isDynamicRoute(route.path)) continue

const url = new URL(route.path, baseURL).toString()
const contentData = await fetchRouteContent(app, route.path, baseURL)
if (contentData) {
htmlMap.set(route.path, contentData)
}
}

return htmlMap
}

interface RouteContent {
content: string | ArrayBuffer
mimeType: string
}

const fetchRouteContent = async <E extends Env = Env, S extends Schema = {}>(
app: Hono<E, S>,
path: string,
baseURL: string
): Promise<RouteContent | null> => {
try {
const url = new URL(path, baseURL).toString()
const response = await app.fetch(new Request(url))
const mimeType = response.headers.get('Content-Type')?.split(';')[0] || 'text/plain'
const content = await parseResponseContent(response)

htmlMap.set(route.path, { content, mimeType })
return { content, mimeType }
} catch (error) {
console.error(`Error fetching content for path: ${path}`, error)
return null
}
}

return htmlMap
const isDynamicRoute = (path: string): boolean => {
return /:\w+|\*/.test(path)
}

/**
Expand Down