From 7b74ec4ba48e363a19d20e322212d0d264927f1b Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Thu, 30 Nov 2023 13:54:15 -0500 Subject: [PATCH] fix(i18n): create fallback pages for page routes correctly (#9252) * add test case * fix(i18n): fallback should create consistent directories (#9142) * fix: index can be 0!! * tests should have the correct configuration --- .changeset/curvy-sheep-lick.md | 6 + packages/astro/src/core/build/generate.ts | 400 +++++++++--------- .../astro/src/core/routing/manifest/create.ts | 27 +- packages/astro/src/i18n/middleware.ts | 6 +- packages/astro/test/i18n-routing.test.js | 41 +- 5 files changed, 263 insertions(+), 217 deletions(-) create mode 100644 .changeset/curvy-sheep-lick.md diff --git a/.changeset/curvy-sheep-lick.md b/.changeset/curvy-sheep-lick.md new file mode 100644 index 0000000000..d369d129d4 --- /dev/null +++ b/.changeset/curvy-sheep-lick.md @@ -0,0 +1,6 @@ +--- +'astro': patch +--- + +Consistently emit fallback routes in the correct folders, and emit routes that +consider `trailingSlash` diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index e35585ca79..5fc439c154 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -10,6 +10,7 @@ import type { GetStaticPathsItem, RouteData, RouteType, + SSRElement, SSRError, SSRLoadedRenderer, SSRManifest, @@ -246,21 +247,24 @@ async function generatePage( builtPaths: Set, pipeline: BuildPipeline ) { - let timeStart = performance.now(); + // prepare information we need const logger = pipeline.getLogger(); const config = pipeline.getConfig(); + const manifest = pipeline.getManifest(); + const pageModulePromise = ssrEntry.page; + const onRequest = ssrEntry.onRequest; const pageInfo = getPageDataByComponent(pipeline.getInternals(), pageData.route.component); - // may be used in the future for handling rel=modulepreload, rel=icon, rel=manifest etc. - const linkIds: [] = []; - const scripts = pageInfo?.hoistedScript ?? null; + // Calculate information of the page, like scripts, links and styles + const hoistedScripts = pageInfo?.hoistedScript ?? null; const styles = pageData.styles .sort(cssOrder) .map(({ sheet }) => sheet) .reduce(mergeInlineCss, []); - - const pageModulePromise = ssrEntry.page; - const onRequest = ssrEntry.onRequest; + // may be used in the future for handling rel=modulepreload, rel=icon, rel=manifest etc. + const linkIds: [] = []; + const scripts = pageInfo?.hoistedScript ?? null; + // prepare the middleware const i18nMiddleware = createI18nMiddleware( pipeline.getManifest().i18n, pipeline.getManifest().base, @@ -289,36 +293,24 @@ async function generatePage( styles, mod: pageModule, }; - - const icon = - pageData.route.type === 'page' || - pageData.route.type === 'redirect' || - pageData.route.type === 'fallback' - ? blue('▶') - : magenta('λ'); - if (isRelativePath(pageData.route.component)) { - logger.info(null, `${icon} ${pageData.route.route}`); - for (const fallbackRoute of pageData.route.fallbackRoutes) { - logger.info(null, `${icon} ${fallbackRoute.route}`); + // Now we explode the routes. A route render itself, and it can render its fallbacks (i18n routing) + for (const route of eachRouteInRouteData(pageData)) { + // Get paths for the route, calling getStaticPaths if needed. + const paths = await getPathsForRoute(route, pageModule, pipeline, builtPaths); + let timeStart = performance.now(); + let prevTimeEnd = timeStart; + for (let i = 0; i < paths.length; i++) { + const path = paths[i]; + pipeline.getEnvironment().logger.debug('build', `Generating: ${path}`); + await generatePath(path, pipeline, generationOptions, route); + const timeEnd = performance.now(); + const timeChange = getTimeStat(prevTimeEnd, timeEnd); + const timeIncrease = `(+${timeChange})`; + const filePath = getOutputFilename(pipeline.getConfig(), path, pageData.route.type); + const lineIcon = i === paths.length - 1 ? '└─' : '├─'; + logger.info(null, ` ${blue(lineIcon)} ${dim(filePath)} ${dim(timeIncrease)}`); + prevTimeEnd = timeEnd; } - } else { - logger.info(null, `${icon} ${pageData.route.component}`); - } - - // Get paths for the route, calling getStaticPaths if needed. - const paths = await getPathsForRoute(pageData, pageModule, pipeline, builtPaths); - - let prevTimeEnd = timeStart; - for (let i = 0; i < paths.length; i++) { - const path = paths[i]; - await generatePath(path, generationOptions, pipeline); - const timeEnd = performance.now(); - const timeChange = getTimeStat(prevTimeEnd, timeEnd); - const timeIncrease = `(${timeChange})`; - const filePath = getOutputFilename(pipeline.getConfig(), path, pageData.route.type); - const lineIcon = i === paths.length - 1 ? '└─' : '├─'; - logger.info(null, ` ${blue(lineIcon)} ${dim(filePath)} ${dim(timeIncrease)}`); - prevTimeEnd = timeEnd; } } @@ -330,7 +322,7 @@ function* eachRouteInRouteData(data: PageBuildData) { } async function getPathsForRoute( - pageData: PageBuildData, + route: RouteData, mod: ComponentInstance, pipeline: BuildPipeline, builtPaths: Set @@ -338,68 +330,62 @@ async function getPathsForRoute( const opts = pipeline.getStaticBuildOptions(); const logger = pipeline.getLogger(); let paths: Array = []; - if (pageData.route.pathname) { - paths.push(pageData.route.pathname); - builtPaths.add(pageData.route.pathname); - for (const virtualRoute of pageData.route.fallbackRoutes) { + if (route.pathname) { + paths.push(route.pathname); + builtPaths.add(route.pathname); + for (const virtualRoute of route.fallbackRoutes) { if (virtualRoute.pathname) { paths.push(virtualRoute.pathname); builtPaths.add(virtualRoute.pathname); } } } else { - for (const route of eachRouteInRouteData(pageData)) { - const staticPaths = await callGetStaticPaths({ - mod, - route, - routeCache: opts.routeCache, - logger, - ssr: isServerLikeOutput(opts.settings.config), - }).catch((err) => { - logger.debug('build', `├── ${bold(red('✗'))} ${route.component}`); - throw err; + const staticPaths = await callGetStaticPaths({ + mod, + route, + routeCache: opts.routeCache, + logger, + ssr: isServerLikeOutput(opts.settings.config), + }).catch((err) => { + logger.debug('build', `├── ${bold(red('✗'))} ${route.component}`); + throw err; + }); + + const label = staticPaths.length === 1 ? 'page' : 'pages'; + logger.debug( + 'build', + `├── ${bold(green('✔'))} ${route.component} → ${magenta(`[${staticPaths.length} ${label}]`)}` + ); + + paths = staticPaths + .map((staticPath) => { + try { + return route.generate(staticPath.params); + } catch (e) { + if (e instanceof TypeError) { + throw getInvalidRouteSegmentError(e, route, staticPath); + } + throw e; + } + }) + .filter((staticPath) => { + // The path hasn't been built yet, include it + if (!builtPaths.has(removeTrailingForwardSlash(staticPath))) { + return true; + } + + // The path was already built once. Check the manifest to see if + // this route takes priority for the final URL. + // NOTE: The same URL may match multiple routes in the manifest. + // Routing priority needs to be verified here for any duplicate + // paths to ensure routing priority rules are enforced in the final build. + const matchedRoute = matchRoute(staticPath, opts.manifest); + return matchedRoute === route; }); - const label = staticPaths.length === 1 ? 'page' : 'pages'; - logger.debug( - 'build', - `├── ${bold(green('✔'))} ${route.component} → ${magenta( - `[${staticPaths.length} ${label}]` - )}` - ); - - paths.push( - ...staticPaths - .map((staticPath) => { - try { - return route.generate(staticPath.params); - } catch (e) { - if (e instanceof TypeError) { - throw getInvalidRouteSegmentError(e, route, staticPath); - } - throw e; - } - }) - .filter((staticPath) => { - // The path hasn't been built yet, include it - if (!builtPaths.has(removeTrailingForwardSlash(staticPath))) { - return true; - } - - // The path was already built once. Check the manifest to see if - // this route takes priority for the final URL. - // NOTE: The same URL may match multiple routes in the manifest. - // Routing priority needs to be verified here for any duplicate - // paths to ensure routing priority rules are enforced in the final build. - const matchedRoute = matchRoute(staticPath, opts.manifest); - return matchedRoute === route; - }) - ); - - // Add each path to the builtPaths set, to avoid building it again later. - for (const staticPath of paths) { - builtPaths.add(removeTrailingForwardSlash(staticPath)); - } + // Add each path to the builtPaths set, to avoid building it again later. + for (const staticPath of paths) { + builtPaths.add(removeTrailingForwardSlash(staticPath)); } } @@ -482,105 +468,126 @@ function getUrlForPath( return url; } -async function generatePath(pathname: string, gopts: GeneratePathOptions, pipeline: BuildPipeline) { - const manifest = pipeline.getManifest(); +interface GeneratePathOptions { + pageData: PageBuildData; + linkIds: string[]; + scripts: { type: 'inline' | 'external'; value: string } | null; + styles: StylesheetAsset[]; + mod: ComponentInstance; +} +async function generatePath( + pathname: string, + pipeline: BuildPipeline, + gopts: GeneratePathOptions, + route: RouteData +) { const { mod, scripts: hoistedScripts, styles: _styles, pageData } = gopts; + const manifest = pipeline.getManifest(); + const logger = pipeline.getLogger(); + pipeline.getEnvironment().logger.debug('build', `Generating: ${pathname}`); - for (const route of eachRouteInRouteData(pageData)) { - // This adds the page name to the array so it can be shown as part of stats. - if (route.type === 'page') { - addPageName(pathname, pipeline.getStaticBuildOptions()); + const links = new Set(); + const scripts = createModuleScriptsSet( + hoistedScripts ? [hoistedScripts] : [], + manifest.base, + manifest.assetsPrefix + ); + const styles = createStylesheetElementSet(_styles, manifest.base, manifest.assetsPrefix); + + if (pipeline.getSettings().scripts.some((script) => script.stage === 'page')) { + const hashedFilePath = pipeline.getInternals().entrySpecifierToBundleMap.get(PAGE_SCRIPT_ID); + if (typeof hashedFilePath !== 'string') { + throw new Error(`Cannot find the built path for ${PAGE_SCRIPT_ID}`); } + const src = createAssetLink(hashedFilePath, manifest.base, manifest.assetsPrefix); + scripts.add({ + props: { type: 'module', src }, + children: '', + }); + } - pipeline.getEnvironment().logger.debug('build', `Generating: ${pathname}`); - - // may be used in the future for handling rel=modulepreload, rel=icon, rel=manifest etc. - const links = new Set(); - const scripts = createModuleScriptsSet( - hoistedScripts ? [hoistedScripts] : [], - manifest.base, - manifest.assetsPrefix - ); - const styles = createStylesheetElementSet(_styles, manifest.base, manifest.assetsPrefix); - - if (pipeline.getSettings().scripts.some((script) => script.stage === 'page')) { - const hashedFilePath = pipeline.getInternals().entrySpecifierToBundleMap.get(PAGE_SCRIPT_ID); - if (typeof hashedFilePath !== 'string') { - throw new Error(`Cannot find the built path for ${PAGE_SCRIPT_ID}`); - } - const src = createAssetLink(hashedFilePath, manifest.base, manifest.assetsPrefix); + // Add all injected scripts to the page. + for (const script of pipeline.getSettings().scripts) { + if (script.stage === 'head-inline') { scripts.add({ - props: { type: 'module', src }, - children: '', + props: {}, + children: script.content, }); } + } - // Add all injected scripts to the page. - for (const script of pipeline.getSettings().scripts) { - if (script.stage === 'head-inline') { - scripts.add({ - props: {}, - children: script.content, - }); - } + const icon = + route.type === 'page' || route.type === 'redirect' || route.type === 'fallback' + ? green('▶') + : magenta('λ'); + if (isRelativePath(route.component)) { + logger.info(null, `${icon} ${route.route}`); + } else { + logger.info(null, `${icon} ${route.component}`); + } + + // This adds the page name to the array so it can be shown as part of stats. + if (route.type === 'page') { + addPageName(pathname, pipeline.getStaticBuildOptions()); + } + + const ssr = isServerLikeOutput(pipeline.getConfig()); + const url = getUrlForPath( + pathname, + pipeline.getConfig().base, + pipeline.getStaticBuildOptions().origin, + pipeline.getConfig().build.format, + route.type + ); + + const request = createRequest({ + url, + headers: new Headers(), + logger: pipeline.getLogger(), + ssr, + }); + const i18n = pipeline.getConfig().experimental.i18n; + + const renderContext = await createRenderContext({ + pathname, + request, + componentMetadata: manifest.componentMetadata, + scripts, + styles, + links, + route, + env: pipeline.getEnvironment(), + mod, + locales: i18n?.locales, + routing: i18n?.routing, + defaultLocale: i18n?.defaultLocale, + }); + + let body: string | Uint8Array; + + let response: Response; + try { + response = await pipeline.renderRoute(renderContext, mod); + } catch (err) { + if (!AstroError.is(err) && !(err as SSRError).id && typeof err === 'object') { + (err as SSRError).id = route.component; } + throw err; + } - const ssr = isServerLikeOutput(pipeline.getConfig()); - const url = getUrlForPath( - pathname, - pipeline.getConfig().base, - pipeline.getStaticBuildOptions().origin, - pipeline.getConfig().build.format, - route.type - ); - - const request = createRequest({ - url, - headers: new Headers(), - logger: pipeline.getLogger(), - ssr, - }); - const i18n = pipeline.getConfig().experimental.i18n; - const renderContext = await createRenderContext({ - pathname, - request, - componentMetadata: manifest.componentMetadata, - scripts, - styles, - links, - route, - env: pipeline.getEnvironment(), - mod, - locales: i18n?.locales, - routing: i18n?.routing, - defaultLocale: i18n?.defaultLocale, - }); - - let body: string | Uint8Array; - - let response: Response; - try { - response = await pipeline.renderRoute(renderContext, mod); - } catch (err) { - if (!AstroError.is(err) && !(err as SSRError).id && typeof err === 'object') { - (err as SSRError).id = pageData.component; - } - throw err; + if (response.status >= 300 && response.status < 400) { + // If redirects is set to false, don't output the HTML + if (!pipeline.getConfig().build.redirects) { + return; } - - if (response.status >= 300 && response.status < 400) { - // If redirects is set to false, don't output the HTML - if (!pipeline.getConfig().build.redirects) { - return; - } - const locationSite = getRedirectLocationOrThrow(response.headers); - const siteURL = pipeline.getConfig().site; - const location = siteURL ? new URL(locationSite, siteURL) : locationSite; - const fromPath = new URL(renderContext.request.url).pathname; - // A short delay causes Google to interpret the redirect as temporary. - // https://developers.google.com/search/docs/crawling-indexing/301-redirects#metarefresh - const delay = response.status === 302 ? 2 : 0; - body = ` + const locationSite = getRedirectLocationOrThrow(response.headers); + const siteURL = pipeline.getConfig().site; + const location = siteURL ? new URL(locationSite, siteURL) : locationSite; + const fromPath = new URL(renderContext.request.url).pathname; + // A short delay causes Google to interpret the redirect as temporary. + // https://developers.google.com/search/docs/crawling-indexing/301-redirects#metarefresh + const delay = response.status === 302 ? 2 : 0; + body = ` Redirecting to: ${location} @@ -588,26 +595,25 @@ async function generatePath(pathname: string, gopts: GeneratePathOptions, pipeli Redirecting from ${fromPath} to ${location} `; - if (pipeline.getConfig().compressHTML === true) { - body = body.replaceAll('\n', ''); - } - // A dynamic redirect, set the location so that integrations know about it. - if (route.type !== 'redirect') { - route.redirect = location.toString(); - } - } else { - // If there's no body, do nothing - if (!response.body) return; - body = Buffer.from(await response.arrayBuffer()); + if (pipeline.getConfig().compressHTML === true) { + body = body.replaceAll('\n', ''); } - - const outFolder = getOutFolder(pipeline.getConfig(), pathname, route.type); - const outFile = getOutFile(pipeline.getConfig(), outFolder, pathname, route.type); - route.distURL = outFile; - - await fs.promises.mkdir(outFolder, { recursive: true }); - await fs.promises.writeFile(outFile, body); + // A dynamic redirect, set the location so that integrations know about it. + if (route.type !== 'redirect') { + route.redirect = location.toString(); + } + } else { + // If there's no body, do nothing + if (!response.body) return; + body = Buffer.from(await response.arrayBuffer()); } + + const outFolder = getOutFolder(pipeline.getConfig(), pathname, route.type); + const outFile = getOutFile(pipeline.getConfig(), outFolder, pathname, route.type); + route.distURL = outFile; + + await fs.promises.mkdir(outFolder, { recursive: true }); + await fs.promises.writeFile(outFile, body); } /** diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index c9a49cbbe4..cfc7a44d8f 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -498,7 +498,7 @@ export function createRouteManifest( const routesByLocale = new Map(); // This type is here only as a helper. We copy the routes and make them unique, so we don't "process" the same route twice. // The assumption is that a route in the file system belongs to only one locale. - const setRoutes = new Set(routes); + const setRoutes = new Set(routes.filter((route) => route.type === 'page')); // First loop // We loop over the locales minus the default locale and add only the routes that contain `/`. @@ -603,22 +603,22 @@ export function createRouteManifest( if (!hasRoute) { let pathname: string | undefined; let route: string; - if (fallbackToLocale === i18n.defaultLocale) { + if ( + fallbackToLocale === i18n.defaultLocale && + i18n.routing === 'prefix-other-locales' + ) { if (fallbackToRoute.pathname) { pathname = `/${fallbackFromLocale}${fallbackToRoute.pathname}`; } route = `/${fallbackFromLocale}${fallbackToRoute.route}`; } else { - pathname = fallbackToRoute.pathname?.replace( - `/${fallbackToLocale}`, - `/${fallbackFromLocale}` - ); - route = fallbackToRoute.route.replace( - `/${fallbackToLocale}`, - `/${fallbackFromLocale}` - ); + pathname = fallbackToRoute.pathname + ?.replace(`/${fallbackToLocale}/`, `/${fallbackFromLocale}/`) + .replace(`/${fallbackToLocale}`, `/${fallbackFromLocale}`); + route = fallbackToRoute.route + .replace(`/${fallbackToLocale}`, `/${fallbackFromLocale}`) + .replace(`/${fallbackToLocale}/`, `/${fallbackFromLocale}/`); } - const segments = removeLeadingForwardSlash(route) .split(path.posix.sep) .filter(Boolean) @@ -626,14 +626,15 @@ export function createRouteManifest( validateSegment(s); return getParts(s, route); }); - + const generate = getRouteGenerator(segments, config.trailingSlash); const index = routes.findIndex((r) => r === fallbackToRoute); - if (index) { + if (index >= 0) { const fallbackRoute: RouteData = { ...fallbackToRoute, pathname, route, segments, + generate, pattern: getPattern(segments, config, config.trailingSlash), type: 'fallback', fallbackRoutes: [], diff --git a/packages/astro/src/i18n/middleware.ts b/packages/astro/src/i18n/middleware.ts index efd9f1e543..f6a3e7cb99 100644 --- a/packages/astro/src/i18n/middleware.ts +++ b/packages/astro/src/i18n/middleware.ts @@ -41,7 +41,7 @@ export function createI18nMiddleware( } const url = context.url; - const { locales, defaultLocale, fallback } = i18n; + const { locales, defaultLocale, fallback, routing } = i18n; const response = await next(); if (response instanceof Response) { @@ -81,8 +81,8 @@ export function createI18nMiddleware( const fallbackLocale = fallback[urlLocale]; let newPathname: string; // If a locale falls back to the default locale, we want to **remove** the locale because - // the default locale doesn't have a prefix, but _only_ if prefix-always is false - if (fallbackLocale === defaultLocale && i18n.routing !== 'prefix-always') { + // the default locale doesn't have a prefix + if (fallbackLocale === defaultLocale && routing === 'prefix-other-locales') { newPathname = url.pathname.replace(`/${urlLocale}`, ``); } else { newPathname = url.pathname.replace(`/${urlLocale}`, `/${fallbackLocale}`); diff --git a/packages/astro/test/i18n-routing.test.js b/packages/astro/test/i18n-routing.test.js index 53f82d16fd..67bba42b18 100644 --- a/packages/astro/test/i18n-routing.test.js +++ b/packages/astro/test/i18n-routing.test.js @@ -672,8 +672,7 @@ describe('[SSG] i18n routing', () => { await fixture.build(); }); - // TODO: enable once we fix fallback - it.skip('should render the en locale', async () => { + it('should render the en locale', async () => { let html = await fixture.readFile('/it/start/index.html'); expect(html).to.include('http-equiv="refresh'); expect(html).to.include('url=/new-site/en/start'); @@ -709,6 +708,40 @@ describe('[SSG] i18n routing', () => { expect(html).to.include('Redirecting to: /en'); }); }); + + describe('i18n routing with fallback and trailing slash', () => { + /** @type {import('./test-utils').Fixture} */ + let fixture; + + before(async () => { + fixture = await loadFixture({ + root: './fixtures/i18n-routing-fallback/', + trailingSlash: 'always', + build: { + format: 'directory', + }, + experimental: { + i18n: { + defaultLocale: 'en', + locales: ['en', 'pt', 'it'], + fallback: { + it: 'en', + }, + routing: { + prefixDefaultLocale: false, + }, + }, + }, + }); + await fixture.build(); + }); + + it('should render the en locale', async () => { + let html = await fixture.readFile('/it/index.html'); + expect(html).to.include('http-equiv="refresh'); + expect(html).to.include('Redirecting to: /new-site/'); + }); + }); }); describe('[SSR] i18n routing', () => { let app; @@ -980,7 +1013,7 @@ describe('[SSR] i18n routing', () => { it: 'en', }, routing: { - prefixDefaultLocale: true, + prefixDefaultLocale: false, }, }, }, @@ -993,7 +1026,7 @@ describe('[SSR] i18n routing', () => { let request = new Request('http://example.com/new-site/it/start'); let response = await app.render(request); expect(response.status).to.equal(302); - expect(response.headers.get('location')).to.equal('/new-site/en/start'); + expect(response.headers.get('location')).to.equal('/new-site/start'); }); }); });