From ee2aca80a71afe843af943b11966fcf77f556cfb Mon Sep 17 00:00:00 2001 From: Happydev <81974850+MoustaphaDev@users.noreply.github.com> Date: Tue, 30 May 2023 14:12:16 +0000 Subject: [PATCH] fix: prioritize dynamic prerendered routes over dynamic server routes (#7235) * test: add unit tests * fix: prioritize prerendered routes * chore: fix test * add comment * test: try avoiding race condition * chore: changeset * try avoiding race conditions attempt #2 * try avoiding race conditions (attempt 3) * final fix hopefuly * tet: add more tests * sort conflicting dynamic routes aplhabetically * test: fix test --- .changeset/hungry-peaches-speak.md | 5 + packages/astro/src/core/render/dev/index.ts | 3 +- .../astro/src/core/routing/manifest/create.ts | 1 + packages/astro/src/prerender/routing.ts | 67 +++++ .../src/vite-plugin-astro-server/route.ts | 22 +- .../test/units/routing/route-matching.test.js | 282 ++++++++++++++++++ 6 files changed, 361 insertions(+), 19 deletions(-) create mode 100644 .changeset/hungry-peaches-speak.md create mode 100644 packages/astro/src/prerender/routing.ts create mode 100644 packages/astro/test/units/routing/route-matching.test.js diff --git a/.changeset/hungry-peaches-speak.md b/.changeset/hungry-peaches-speak.md new file mode 100644 index 000000000000..4467b521dc4b --- /dev/null +++ b/.changeset/hungry-peaches-speak.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Prioritize dynamic prerendered routes over dynamic server routes diff --git a/packages/astro/src/core/render/dev/index.ts b/packages/astro/src/core/render/dev/index.ts index 26e7c85d53b5..c0113739285a 100644 --- a/packages/astro/src/core/render/dev/index.ts +++ b/packages/astro/src/core/render/dev/index.ts @@ -1,4 +1,3 @@ -import { fileURLToPath } from 'url'; import type { AstroMiddlewareInstance, AstroSettings, @@ -65,7 +64,7 @@ export async function preload({ try { // Load the module from the Vite SSR Runtime. - const mod = (await env.loader.import(fileURLToPath(filePath))) as ComponentInstance; + const mod = (await env.loader.import(viteID(filePath))) as ComponentInstance; return [renderers, mod]; } catch (error) { diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index a673e199ba2f..a8680a104d46 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -173,6 +173,7 @@ function comparator(a: Item, b: Item) { } } + // endpoints are prioritized over pages if (a.isPage !== b.isPage) { return a.isPage ? 1 : -1; } diff --git a/packages/astro/src/prerender/routing.ts b/packages/astro/src/prerender/routing.ts new file mode 100644 index 000000000000..92878904400c --- /dev/null +++ b/packages/astro/src/prerender/routing.ts @@ -0,0 +1,67 @@ +import type { AstroSettings, RouteData } from '../@types/astro'; +import { preload, type DevelopmentEnvironment } from '../core/render/dev/index.js'; +import { getPrerenderStatus } from './metadata.js'; + +type GetSortedPreloadedMatchesParams = { + env: DevelopmentEnvironment; + matches: RouteData[]; + settings: AstroSettings; +}; +export async function getSortedPreloadedMatches({ + env, + matches, + settings, +}: GetSortedPreloadedMatchesParams) { + return ( + await preloadAndSetPrerenderStatus({ + env, + matches, + settings, + }) + ).sort((a, b) => prioritizePrerenderedMatchesComparator(a.route, b.route)); +} + +type PreloadAndSetPrerenderStatusParams = { + env: DevelopmentEnvironment; + matches: RouteData[]; + settings: AstroSettings; +}; +async function preloadAndSetPrerenderStatus({ + env, + matches, + settings, +}: PreloadAndSetPrerenderStatusParams) { + const preloaded = await Promise.all( + matches.map(async (route) => { + const filePath = new URL(`./${route.component}`, settings.config.root); + const preloadedComponent = await preload({ env, filePath }); + + // gets the prerender metadata set by the `astro:scanner` vite plugin + const prerenderStatus = getPrerenderStatus({ + filePath, + loader: env.loader, + }); + + if (prerenderStatus !== undefined) { + route.prerender = prerenderStatus; + } + + return { preloadedComponent, route, filePath } as const; + }) + ); + return preloaded; +} + +function prioritizePrerenderedMatchesComparator(a: RouteData, b: RouteData): number { + if (areRegexesEqual(a.pattern, b.pattern)) { + if (a.prerender !== b.prerender) { + return a.prerender ? -1 : 1; + } + return a.component < b.component ? -1 : 1; + } + return 0; +} + +function areRegexesEqual(regexp1: RegExp, regexp2: RegExp) { + return regexp1.source === regexp2.source && regexp1.global === regexp2.global; +} diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 86b8e5814e46..56fd60f14eca 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -16,10 +16,10 @@ import { preload, renderPage } from '../core/render/dev/index.js'; import { getParamsAndProps, GetParamsAndPropsError } from '../core/render/index.js'; import { createRequest } from '../core/request.js'; import { matchAllRoutes } from '../core/routing/index.js'; -import { getPrerenderStatus } from '../prerender/metadata.js'; import { isHybridOutput } from '../prerender/utils.js'; import { log404 } from './common.js'; import { handle404Response, writeSSRResult, writeWebResponse } from './response.js'; +import { getSortedPreloadedMatches } from '../prerender/routing.js'; type AsyncReturnType Promise> = T extends ( ...args: any @@ -47,24 +47,12 @@ export async function matchRoute( ): Promise { const { logging, settings, routeCache } = env; const matches = matchAllRoutes(pathname, manifest); + const preloadedMatches = await getSortedPreloadedMatches({ env, matches, settings }); - for await (const maybeRoute of matches) { - const filePath = new URL(`./${maybeRoute.component}`, settings.config.root); - const preloadedComponent = await preload({ env, filePath }); - - // gets the prerender metadata set by the `astro:scanner` vite plugin - const prerenderStatus = getPrerenderStatus({ - filePath, - loader: env.loader, - }); - - if (prerenderStatus !== undefined) { - maybeRoute.prerender = prerenderStatus; - } - - const [, mod] = preloadedComponent; + for await (const { preloadedComponent, route: maybeRoute, filePath } of preloadedMatches) { // attempt to get static paths // if this fails, we have a bad URL match! + const [, mod] = preloadedComponent; const paramsAndPropsRes = await getParamsAndProps({ mod, route: maybeRoute, @@ -210,7 +198,7 @@ export async function handleRoute( await writeWebResponse(res, result.response); } else { let contentType = 'text/plain'; - // Dynamic routes don’t include `route.pathname`, so synthesize a path for these (e.g. 'src/pages/[slug].svg') + // Dynamic routes don't include `route.pathname`, so synthesize a path for these (e.g. 'src/pages/[slug].svg') const filepath = route.pathname || route.segments.map((segment) => segment.map((p) => p.content).join('')).join('/'); diff --git a/packages/astro/test/units/routing/route-matching.test.js b/packages/astro/test/units/routing/route-matching.test.js new file mode 100644 index 000000000000..692518382836 --- /dev/null +++ b/packages/astro/test/units/routing/route-matching.test.js @@ -0,0 +1,282 @@ +// @ts-check +import { createFs, createRequestAndResponse } from '../test-utils.js'; +import { createRouteManifest, matchAllRoutes } from '../../../dist/core/routing/index.js'; +import { fileURLToPath } from 'url'; +import { defaultLogging } from '../../test-utils.js'; +import { createViteLoader } from '../../../dist/core/module-loader/vite.js'; +import { createDevelopmentEnvironment } from '../../../dist/core/render/dev/environment.js'; +import { expect } from 'chai'; +import { createContainer } from '../../../dist/core/dev/container.js'; +import * as cheerio from 'cheerio'; +import testAdapter from '../../test-adapter.js'; +import { getSortedPreloadedMatches } from '../../../dist/prerender/routing.js'; + +const root = new URL('../../fixtures/alias/', import.meta.url); +const fileSystem = { + '/src/pages/[serverDynamic].astro': ` + --- + export const prerender = false; + --- +

Server dynamic route! slug:{Astro.params.serverDynamic}

+ `, + + '/src/pages/[xStaticDynamic].astro': ` + --- + export function getStaticPaths() { + return [ + { + params: { + xStaticDynamic: "static-dynamic-route-here", + }, + }, + ]; + } + --- +

Prerendered dynamic route!

+ `, + '/src/pages/[aStaticDynamic].astro': ` + --- + export function getStaticPaths() { + return [ + { + params: { + aStaticDynamic: "another-static-dynamic-route-here", + }, + }, + ]; + } + --- +

Another prerendered dynamic route!

+ `, + '/src/pages/[...serverRest].astro': ` + --- + export const prerender = false; + --- +

Server rest route! slug:{Astro.params.serverRest}

+ `, + '/src/pages/[...xStaticRest].astro': ` + --- + export function getStaticPaths() { + return [ + { + params: { + xStaticRest: undefined, + }, + }, + ]; + } + --- +

Prerendered rest route!

+`, + '/src/pages/[...aStaticRest].astro': ` + --- + export function getStaticPaths() { + return [ + { + params: { + aStaticRest: "another/static-rest-route-here", + }, + }, + ]; + } + --- +

Another prerendered rest route!

+`, + + '/src/pages/nested/[...serverRest].astro': ` + --- + export const prerender = false; + --- +

Nested server rest route! slug: {Astro.params.serverRest}

+ `, + '/src/pages/nested/[...xStaticRest].astro': ` + --- + export function getStaticPaths() { + return [ + { + params: { + xStaticRest: undefined, + }, + }, + ]; + } + --- +

Nested prerendered rest route!

+`, + '/src/pages/nested/[...aStaticRest].astro': ` + --- + export function getStaticPaths() { + return [ + { + params: { + aStaticRest: "another-nested-static-dynamic-rest-route-here", + }, + }, + ]; + } + --- +

Another nested prerendered rest route!

+`, +}; + +describe('Route matching', () => { + let env; + let manifest; + let container; + let settings; + + before(async () => { + const fs = createFs(fileSystem, root); + container = await createContainer({ + fs, + root, + userConfig: { + trailingSlash: 'never', + output: 'hybrid', + experimental: { + hybridOutput: true, + }, + adapter: testAdapter(), + }, + disableTelemetry: true, + }); + settings = container.settings; + + const loader = createViteLoader(container.viteServer); + env = createDevelopmentEnvironment(container.settings, defaultLogging, loader); + manifest = createRouteManifest( + { + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }, + defaultLogging + ); + }); + + after(async () => { + await container.close(); + }); + + describe('Matched routes', () => { + it('should be sorted correctly', async () => { + const matches = matchAllRoutes('/try-matching-a-route', manifest); + const preloadedMatches = await getSortedPreloadedMatches({ env, matches, settings }); + const sortedRouteNames = preloadedMatches.map((match) => match.route.route); + + expect(sortedRouteNames).to.deep.equal([ + '/[astaticdynamic]', + '/[xstaticdynamic]', + '/[serverdynamic]', + '/[...astaticrest]', + '/[...xstaticrest]', + '/[...serverrest]', + ]); + }); + it('nested should be sorted correctly', async () => { + const matches = matchAllRoutes('/nested/try-matching-a-route', manifest); + const preloadedMatches = await getSortedPreloadedMatches({ env, matches, settings }); + const sortedRouteNames = preloadedMatches.map((match) => match.route.route); + + expect(sortedRouteNames).to.deep.equal([ + '/nested/[...astaticrest]', + '/nested/[...xstaticrest]', + '/nested/[...serverrest]', + '/[...astaticrest]', + '/[...xstaticrest]', + '/[...serverrest]', + ]); + }); + }); + + describe('Request', () => { + it('should correctly match a static dynamic route I', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '/static-dynamic-route-here', + }); + container.handle(req, res); + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Prerendered dynamic route!'); + }); + + it('should correctly match a static dynamic route II', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '/another-static-dynamic-route-here', + }); + container.handle(req, res); + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Another prerendered dynamic route!'); + }); + + it('should correctly match a server dynamic route', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '/a-random-slug-was-matched', + }); + container.handle(req, res); + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Server dynamic route! slug:a-random-slug-was-matched'); + }); + + it('should correctly match a static rest route I', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '', + }); + container.handle(req, res); + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Prerendered rest route!'); + }); + + it('should correctly match a static rest route II', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '/another/static-rest-route-here', + }); + container.handle(req, res); + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Another prerendered rest route!'); + }); + + it('should correctly match a nested static rest route index', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '/nested', + }); + container.handle(req, res); + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Nested prerendered rest route!'); + }); + + it('should correctly match a nested static rest route', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '/nested/another-nested-static-dynamic-rest-route-here', + }); + container.handle(req, res); + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Another nested prerendered rest route!'); + }); + + it('should correctly match a nested server rest route', async () => { + const { req, res, text } = createRequestAndResponse({ + method: 'GET', + url: '/nested/a-random-slug-was-matched', + }); + container.handle(req, res); + + const html = await text(); + const $ = cheerio.load(html); + expect($('p').text()).to.equal('Nested server rest route! slug: a-random-slug-was-matched'); + }); + }); +});