michael/astro
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Fixes for redirects config (#7644)

Browse source 
* Update redirects static generation based on recs

Got some great recommendations on how to handle our HTML written
redirect code based on SEO best practices.

See https://github.com/withastro/roadmap/issues/466#issuecomment-1595940678

This implements them all.

* Fix for using the root path / as a redirect

Fixes https://github.com/withastro/astro/issues/7478

* Fix static redirects prefer over dynamic page

Fixes https://github.com/withastro/astro/issues/7581
This commit is contained in:
Matthew Phillips 2023-07-13 16:09:44 -04:00 • committed by GitHub
parent af5827d4f7
commit 213e10991a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 110 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
.changeset/four-pumpkins-try.md Normal file
View file

@ -0,0 +1,5 @@
---
'astro': patch
---
Fix for allowing the root path / as a redirect

5
.changeset/light-eggs-hug.md Normal file
View file

@ -0,0 +1,5 @@
---
'astro': patch
---
Fix static redirects prefered over dynamic regular routes

11
packages/astro/src/core/build/generate.ts
View file

@ -619,9 +619,18 @@ async function generatePath(
return; return;
} }
const location = getRedirectLocationOrThrow(response.headers); const location = getRedirectLocationOrThrow(response.headers);
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 = `<!doctype html> body = `<!doctype html>
<title>Redirecting to: ${location}</title> <title>Redirecting to: ${location}</title>
<meta http-equiv="refresh" content="0;url=${location}" />`; <meta http-equiv="refresh" content="${delay};url=${location}">
<meta name="robots" content="noindex">
<link rel="canonical" href="${location}">
<body>
<a href="${location}">Redirecting from <code>${fromPath}</code> to <code>${location}</code></a>
</body>`;
// A dynamic redirect, set the location so that integrations know about it. // A dynamic redirect, set the location so that integrations know about it.
if (pageData.route.type !== 'redirect') { if (pageData.route.type !== 'redirect') {
pageData.route.redirect = location; pageData.route.redirect = location;

7
packages/astro/src/core/build/static-build.ts
View file

@ -32,6 +32,7 @@ import { RESOLVED_SPLIT_MODULE_ID, SSR_VIRTUAL_MODULE_ID } from './plugins/plugi
import { ASTRO_PAGE_EXTENSION_POST_PATTERN } from './plugins/util.js'; import { ASTRO_PAGE_EXTENSION_POST_PATTERN } from './plugins/util.js';
import type { PageBuildData, StaticBuildOptions } from './types'; import type { PageBuildData, StaticBuildOptions } from './types';
import { getTimeStat } from './util.js'; import { getTimeStat } from './util.js';
import { routeIsRedirect } from '../redirects/index.js';
export async function viteBuild(opts: StaticBuildOptions) { export async function viteBuild(opts: StaticBuildOptions) {
const { allPages, settings } = opts; const { allPages, settings } = opts;
@ -60,8 +61,10 @@ export async function viteBuild(opts: StaticBuildOptions) {
// Track the page data in internals // Track the page data in internals
trackPageData(internals, component, pageData, astroModuleId, astroModuleURL); trackPageData(internals, component, pageData, astroModuleId, astroModuleURL);
pageInput.add(astroModuleId); if(!routeIsRedirect(pageData.route)) {
facadeIdToPageDataMap.set(fileURLToPath(astroModuleURL), pageData); pageInput.add(astroModuleId);
facadeIdToPageDataMap.set(fileURLToPath(astroModuleURL), pageData);
}
} }
// Empty out the dist folder, if needed. Vite has a config for doing this // Empty out the dist folder, if needed. Vite has a config for doing this

23
packages/astro/src/core/routing/manifest/create.ts
View file

@ -458,7 +458,28 @@ export function createRouteManifest(
redirectRoute: routes.find((r) => r.route === to), redirectRoute: routes.find((r) => r.route === to),
}; };
// Push so that redirects are selected last. const lastSegmentIsDynamic = (r: RouteData) => !!r.segments.at(-1)?.at(-1)?.dynamic;
const redirBase = path.posix.dirname(route);
const dynamicRedir = lastSegmentIsDynamic(routeData);
let i = 0;
for(const existingRoute of routes) {
// An exact match, prefer the page/endpoint. This matches hosts.
if(existingRoute.route === route) {
routes.splice(i+1, 0, routeData);
return;
}
// If the existing route is dynamic, prefer the static redirect.
const base = path.posix.dirname(existingRoute.route);
if(base === redirBase && !dynamicRedir && lastSegmentIsDynamic(existingRoute)) {
routes.splice(i, 0, routeData);
return;
}
i++;
}
// Didn't find a good place, insert last
routes.push(routeData); routes.push(routeData);
}); });

2
packages/astro/test/fixtures/ssr-redirect/src/middleware.ts vendored
View file

@ -5,7 +5,7 @@ export const onRequest = defineMiddleware(({ request }, next) => {
return new Response(null, { return new Response(null, {
status: 301, status: 301,
headers: { headers: {
'Location': '/' 'Location': '/test'
} }
}); });
} }

0
packages/astro/test/fixtures/ssr-redirect/src/pages/index.astro → packages/astro/test/fixtures/ssr-redirect/src/pages/test.astro vendored
View file

43
packages/astro/test/redirects.test.js
View file

@ -13,7 +13,7 @@ describe('Astro.redirect', () => {
output: 'server', output: 'server',
adapter: testAdapter(), adapter: testAdapter(),
redirects: { redirects: {
'/api/redirect': '/', '/api/redirect': '/test',
}, },
experimental: { experimental: {
redirects: true, redirects: true,
@ -75,12 +75,13 @@ describe('Astro.redirect', () => {
redirects: true, redirects: true,
}, },
redirects: { redirects: {
'/one': '/', '/': '/test',
'/two': '/', '/one': '/test',
'/two': '/test',
'/blog/[...slug]': '/articles/[...slug]', '/blog/[...slug]': '/articles/[...slug]',
'/three': { '/three': {
status: 302, status: 302,
destination: '/', destination: '/test',
}, },
}, },
}); });
@ -93,18 +94,44 @@ describe('Astro.redirect', () => {
expect(html).to.include('url=/login'); expect(html).to.include('url=/login');
}); });
it('Includes the meta noindex tag', async () => {
const html = await fixture.readFile('/secret/index.html');
expect(html).to.include('name="robots');
expect(html).to.include('content="noindex');
});
it('Includes a link to the new pages for bots to follow', async () => {
const html = await fixture.readFile('/secret/index.html');
expect(html).to.include('<a href="/login">');
});
it('Includes a canonical link', async () => {
const html = await fixture.readFile('/secret/index.html');
expect(html).to.include('<link rel="canonical" href="/login">');
});
it('A 302 status generates a "temporary redirect" through a short delay', async () => {
// https://developers.google.com/search/docs/crawling-indexing/301-redirects#metarefresh
const html = await fixture.readFile('/secret/index.html');
expect(html).to.include('content="2;url=/login"');
});
it('Includes the meta refresh tag in `redirect` config pages', async () => { it('Includes the meta refresh tag in `redirect` config pages', async () => {
let html = await fixture.readFile('/one/index.html'); let html = await fixture.readFile('/one/index.html');
expect(html).to.include('http-equiv="refresh'); expect(html).to.include('http-equiv="refresh');
expect(html).to.include('url=/'); expect(html).to.include('url=/test');
html = await fixture.readFile('/two/index.html'); html = await fixture.readFile('/two/index.html');
expect(html).to.include('http-equiv="refresh'); expect(html).to.include('http-equiv="refresh');
expect(html).to.include('url=/'); expect(html).to.include('url=/test');
html = await fixture.readFile('/three/index.html'); html = await fixture.readFile('/three/index.html');
expect(html).to.include('http-equiv="refresh'); expect(html).to.include('http-equiv="refresh');
expect(html).to.include('url=/'); expect(html).to.include('url=/test');
html = await fixture.readFile('/index.html');
expect(html).to.include('http-equiv="refresh');
expect(html).to.include('url=/test');
}); });
it('Generates page for dynamic routes', async () => { it('Generates page for dynamic routes', async () => {
@ -120,7 +147,7 @@ describe('Astro.redirect', () => {
it('Generates redirect pages for redirects created by middleware', async () => { it('Generates redirect pages for redirects created by middleware', async () => {
let html = await fixture.readFile('/middleware-redirect/index.html'); let html = await fixture.readFile('/middleware-redirect/index.html');
expect(html).to.include('http-equiv="refresh'); expect(html).to.include('http-equiv="refresh');
expect(html).to.include('url=/'); expect(html).to.include('url=/test');
}); });
}); });

28
packages/astro/test/units/routing/manifest.test.js
View file

@ -4,6 +4,7 @@ import { createFs } from '../test-utils.js';
import { createRouteManifest } from '../../../dist/core/routing/manifest/create.js'; import { createRouteManifest } from '../../../dist/core/routing/manifest/create.js';
import { createDefaultDevSettings } from '../../../dist/core/config/index.js'; import { createDefaultDevSettings } from '../../../dist/core/config/index.js';
import { fileURLToPath } from 'url'; import { fileURLToPath } from 'url';
import { defaultLogging } from '../test-utils.js';
const root = new URL('../../fixtures/alias/', import.meta.url); const root = new URL('../../fixtures/alias/', import.meta.url);
@ -59,6 +60,31 @@ describe('routing - createRouteManifest', () => {
expect(manifest.routes[1].route).to.equal('/blog/contributing'); expect(manifest.routes[1].route).to.equal('/blog/contributing');
expect(manifest.routes[1].type).to.equal('page'); expect(manifest.routes[1].type).to.equal('page');
expect(manifest.routes[2].route).to.equal('/blog/[...slug]'); expect(manifest.routes[3].route).to.equal('/blog/[...slug]');
});
it('static redirect route is prioritized over dynamic file route', async () => {
const fs = createFs(
{
'/src/pages/[...slug].astro': `<h1>test</h1>`
},
root
);
const settings = await createDefaultDevSettings(
{
trailingSlash: 'never',
redirects: {
'/foo': '/bar'
}
},
root
);
const manifest = createRouteManifest({
cwd: fileURLToPath(root),
settings,
fsMod: fs,
}, defaultLogging);
expect(manifest.routes[0].route).to.equal('/foo');
}); });
}); });