Updates the dev server to handle multiple routes matching the same URL (#4087)

* updates the dev server to handle multiple routes matching the same URL

* Adding an error message when multiple routes match in SSR

* resetting the flag used by the `calledTwiceTest`

* injected routes should be sorted with a higher priority than user-defined routes

* adding routing priority tests for injected routes

* chore: add changeset

* adding a dev test to make sure getStaticPaths is called once
This commit is contained in:
Tony Sullivan 2022-07-29 20:54:51 +00:00 committed by GitHub
parent 8e20156638
commit a0d1731a7e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 393 additions and 212 deletions

View file

@ -0,0 +1,5 @@
---
'astro': patch
---
Fixes a couple routing bugs that could lead to routing differences in `dev` vs. `build` when using multiple dynamic routes

View file

@ -1,5 +1,5 @@
export { createRouteManifest } from './manifest/create.js';
export { deserializeRouteData, serializeRouteData } from './manifest/serialization.js';
export { matchRoute } from './match.js';
export { matchRoute, matchAllRoutes } from './match.js';
export { getParams } from './params.js';
export { validateGetStaticPathsModule, validateGetStaticPathsResult } from './validation.js';

View file

@ -1,4 +1,4 @@
import type { AstroConfig, ManifestData, RouteData, RoutePart } from '../../../@types/astro';
import type { AstroConfig, InjectedRoute, ManifestData, RouteData, RoutePart } from '../../../@types/astro';
import type { LogOptions } from '../../logger/core';
import fs from 'fs';
@ -159,6 +159,29 @@ function comparator(a: Item, b: Item) {
return a.file < b.file ? -1 : 1;
}
function injectedRouteToItem(
{ config, cwd }: { config: AstroConfig; cwd?: string },
{ pattern, entryPoint }: InjectedRoute
): Item {
const resolved = require.resolve(entryPoint, { paths: [cwd || fileURLToPath(config.root)] });
const ext = path.extname(pattern);
const type = resolved.endsWith('.astro') ? 'page' : 'endpoint';
const isPage = type === 'page';
return {
basename: pattern,
ext,
parts: getParts(pattern, resolved),
file: resolved,
isDir: false,
isIndex: true,
isPage,
routeSuffix: pattern.slice(pattern.indexOf('.'), -ext.length),
}
}
/** Create manifest of all static routes */
export function createRouteManifest(
{ config, cwd }: { config: AstroConfig; cwd?: string },
@ -288,7 +311,14 @@ export function createRouteManifest(
warn(logging, 'astro', `Missing pages directory: ${pagesDirRootRelative}`);
}
config?._ctx?.injectedRoutes?.forEach(({ pattern: name, entryPoint }) => {
config?._ctx?.injectedRoutes?.sort((a, b) =>
// sort injected routes in the same way as user-defined routes
comparator(
injectedRouteToItem({ config, cwd }, a),
injectedRouteToItem({ config, cwd}, b)
))
.reverse() // prepend to the routes array from lowest to highest priority
.forEach(({ pattern: name, entryPoint }) => {
const resolved = require.resolve(entryPoint, { paths: [cwd || fileURLToPath(config.root)] });
const component = slash(path.relative(cwd || fileURLToPath(config.root), resolved));
@ -336,7 +366,10 @@ export function createRouteManifest(
);
}
routes.push({
// the routes array was already sorted by priority,
// pushing to the front of the list ensure that injected routes
// are given priority over all user-provided routes
routes.unshift({
type,
route,
pattern,

View file

@ -4,3 +4,8 @@ import type { ManifestData, RouteData } from '../../@types/astro';
export function matchRoute(pathname: string, manifest: ManifestData): RouteData | undefined {
return manifest.routes.find((route) => route.pattern.test(pathname));
}
/** Finds all matching routes from pathname */
export function matchAllRoutes(pathname: string, manifest: ManifestData): RouteData[] {
return manifest.routes.filter((route) => route.pattern.test(pathname));
}

View file

@ -19,7 +19,7 @@ import { getParamsAndProps, GetParamsAndPropsError } from '../core/render/core.j
import { preload, ssr } from '../core/render/dev/index.js';
import { RouteCache } from '../core/render/route-cache.js';
import { createRequest } from '../core/request.js';
import { createRouteManifest, matchRoute } from '../core/routing/index.js';
import { createRouteManifest, matchAllRoutes, matchRoute } from '../core/routing/index.js';
import { createSafeError, resolvePages } from '../core/util.js';
import notFoundTemplate, { subpathNotUsedTemplate } from '../template/4xx.js';
@ -248,26 +248,81 @@ async function handleRequest(
clientAddress: buildingToSSR ? req.socket.remoteAddress : undefined,
});
let filePath: URL | undefined;
try {
// Attempt to match the URL to a valid page route.
// If that fails, switch the response to a 404 response.
let route = matchRoute(pathname, manifest);
const statusCode = route ? 200 : 404;
async function matchRoute() {
const matches = matchAllRoutes(pathname, manifest);
if (!route) {
log404(logging, pathname);
const custom404 = getCustom404Route(config, manifest);
if (custom404) {
route = custom404;
} else {
return handle404Response(origin, config, req, res);
if (config.output === 'server' && matches.length > 1) {
throw new Error(`Found multiple matching routes for "${pathname}"! When using \`output: 'server'\`, only one route in \`src/pages\` can match a given URL. Found:
${
matches.map(({ component }) => `- ${component}`).join('\n')
}
`);
}
for await (const maybeRoute of matches) {
const filePath = new URL(`./${maybeRoute.component}`, config.root);
const preloadedComponent = await preload({ astroConfig: config, filePath, viteServer });
const [, mod] = preloadedComponent;
// attempt to get static paths
// if this fails, we have a bad URL match!
const paramsAndPropsRes = await getParamsAndProps({
mod,
route: maybeRoute,
routeCache,
pathname: pathname,
logging,
ssr: config.output === 'server',
});
if (paramsAndPropsRes !== GetParamsAndPropsError.NoMatchingStaticPath) {
return {
route: maybeRoute,
filePath,
preloadedComponent,
mod,
}
}
}
filePath = new URL(`./${route.component}`, config.root);
const preloadedComponent = await preload({ astroConfig: config, filePath, viteServer });
const [, mod] = preloadedComponent;
if (matches.length) {
warn(
logging,
'getStaticPaths',
`Route pattern matched, but no matching static path found. (${pathname})`
);
}
log404(logging, pathname);
const custom404 = getCustom404Route(config, manifest);
if (custom404) {
const filePath = new URL(`./${custom404.component}`, config.root);
const preloadedComponent = await preload({ astroConfig: config, filePath, viteServer });
const [, mod] = preloadedComponent;
return {
route: custom404,
filePath,
preloadedComponent,
mod
};
}
return undefined;
}
let filePath: URL | undefined;
try {
const matchedRoute = await matchRoute();
if (!matchedRoute) {
return handle404Response(origin, config, req, res);
}
const { route, preloadedComponent, mod } = matchedRoute;
filePath = matchedRoute.filePath;
// attempt to get static paths
// if this fails, we have a bad URL match!
const paramsAndPropsRes = await getParamsAndProps({
@ -278,38 +333,6 @@ async function handleRequest(
logging,
ssr: config.output === 'server',
});
if (paramsAndPropsRes === GetParamsAndPropsError.NoMatchingStaticPath) {
warn(
logging,
'getStaticPaths',
`Route pattern matched, but no matching static path found. (${pathname})`
);
log404(logging, pathname);
const routeCustom404 = getCustom404Route(config, manifest);
if (routeCustom404) {
const filePathCustom404 = new URL(`./${routeCustom404.component}`, config.root);
const preloadedCompCustom404 = await preload({
astroConfig: config,
filePath: filePathCustom404,
viteServer,
});
const result = await ssr(preloadedCompCustom404, {
astroConfig: config,
filePath: filePathCustom404,
logging,
mode: 'development',
origin,
pathname: pathname,
request,
route: routeCustom404,
routeCache,
viteServer,
});
return await writeSSRResult(result, res);
} else {
return handle404Response(origin, config, req, res);
}
}
const options: SSROptions = {
astroConfig: config,

View file

@ -4,6 +4,9 @@ import * as cheerio from 'cheerio';
describe('getStaticPaths - build calls', () => {
before(async () => {
// reset the flag used by [...calledTwiceTest].astro between each test
globalThis.isCalledOnce = false;
const fixture = await loadFixture({
root: './fixtures/astro-get-static-paths/',
site: 'https://mysite.dev/',
@ -11,17 +14,49 @@ describe('getStaticPaths - build calls', () => {
});
await fixture.build();
});
it('is only called once during build', () => {
// useless expect; if build() throws in setup then this test fails
expect(true).to.equal(true);
});
});
describe('getStaticPaths - dev calls', () => {
let fixture;
let devServer;
before(async () => {
// reset the flag used by [...calledTwiceTest].astro between each test
globalThis.isCalledOnce = false;
fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/' });
devServer = await fixture.startDevServer();
});
after(async () => {
devServer.stop();
});
it('only calls getStaticPaths once', async () => {
let res = await fixture.fetch('/a');
expect(res.status).to.equal(200);
res = await fixture.fetch('/b');
expect(res.status).to.equal(200);
res = await fixture.fetch('/c');
expect(res.status).to.equal(200);
});
});
describe('getStaticPaths - 404 behavior', () => {
let fixture;
let devServer;
before(async () => {
// reset the flag used by [...calledTwiceTest].astro between each test
globalThis.isCalledOnce = false;
fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/' });
devServer = await fixture.startDevServer();
});
@ -55,6 +90,9 @@ describe('getStaticPaths - route params type validation', () => {
let fixture, devServer;
before(async () => {
// reset the flag used by [...calledTwiceTest].astro between each test
globalThis.isCalledOnce = false;
fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/' });
devServer = await fixture.startDevServer();
});
@ -81,6 +119,9 @@ describe('getStaticPaths - numeric route params', () => {
let devServer;
before(async () => {
// reset the flag used by [...calledTwiceTest].astro between each test
globalThis.isCalledOnce = false;
fixture = await loadFixture({
root: './fixtures/astro-get-static-paths/',
site: 'https://mysite.dev/',

View file

@ -1,4 +1,7 @@
import { defineConfig } from 'astro/config';
import integration from './integration.mjs';
// https://astro.build/config
export default defineConfig({});
export default defineConfig({
integrations: [integration()]
});

View file

@ -0,0 +1,21 @@
export default function() {
return {
name: '@astrojs/test-integration',
hooks: {
'astro:config:setup': ({ injectRoute }) => {
injectRoute({
pattern: '/injected',
entryPoint: './src/to-inject.astro'
});
injectRoute({
pattern: '/_injected',
entryPoint: './src/_to-inject.astro'
});
injectRoute({
pattern: '/[id]',
entryPoint: './src/[id].astro'
});
}
}
}
}

View file

@ -0,0 +1,21 @@
---
export async function getStaticPaths() {
return [
{ params: { id: 'injected-1' } },
{ params: { id: 'injected-2' } }
];
}
const { id } = Astro.params;
---
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width" />
<title>Routing</title>
</head>
<body>
<h1>[id].astro</h1>
<p>{id}</p>
</body>
</html>

View file

@ -0,0 +1,12 @@
---
---
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width" />
<title>Routing</title>
</head>
<body>
<h1>to-inject.astro</h1>
</body>
</html>

View file

@ -0,0 +1,27 @@
---
export async function getStaticPaths() {
return [
{
params: { page: "page-1" }
},
{
params: { page: "page-2" }
}
]
}
---
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width" />
<title>Routing</title>
</head>
<body>
<h1>[page].astro</h1>
<p>{Astro.params.page}</p>
</body>
</html>

View file

@ -0,0 +1,27 @@
---
export async function getStaticPaths() {
return [
{
params: { slug: "slug-1" },
},
{
params: { slug: "slug-2" },
}
]
}
---
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width" />
<title>Routing</title>
</head>
<body>
<h1>[slug].astro</h1>
<p>{Astro.params.slug}</p>
</body>
</html>

View file

@ -0,0 +1,12 @@
---
---
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width" />
<title>Routing</title>
</head>
<body>
<h1>to-inject.astro</h1>
</body>
</html>

View file

@ -1,140 +1,140 @@
import { expect } from 'chai';
import { load as cheerioLoad } from 'cheerio';
import path from 'path';
import { loadFixture } from './test-utils.js';
let fixture;
const routes = [
{
description: 'matches / to index.astro',
url: '/',
h1: 'index.astro',
},
{
description: 'matches /slug-1 to [slug].astro',
url: '/slug-1',
h1: '[slug].astro',
p: 'slug-1',
},
{
description: 'matches /slug-2 to [slug].astro',
url: '/slug-2',
h1: '[slug].astro',
p: 'slug-2',
},
{
description: 'matches /page-1 to [page].astro',
url: '/page-1',
h1: '[page].astro',
p: 'page-1',
},
{
description: 'matches /page-2 to [page].astro',
url: '/page-2',
h1: '[page].astro',
p: 'page-2',
},
{
description: 'matches /posts/post-1 to posts/[pid].astro',
url: '/posts/post-1',
h1: 'posts/[pid].astro',
p: 'post-1',
},
{
description: 'matches /posts/post-2 to posts/[pid].astro',
url: '/posts/post-2',
h1: 'posts/[pid].astro',
p: 'post-2',
},
{
description: 'matches /posts/1/2 to posts/[...slug].astro',
url: '/posts/1/2',
h1: 'posts/[...slug].astro',
p: '1/2',
},
{
description: 'matches /de to de/index.astro',
url: '/de',
h1: 'de/index.astro',
},
{
url: '/de/',
h1: 'de/index.astro',
},
{
url: '/de/index.html',
h1: 'de/index.astro',
h1: 'de/index.astro (priority)',
},
{
description: 'matches /en to [lang]/index.astro',
url: '/en',
h1: '[lang]/index.astro',
p: 'en',
},
{
url: '/en/',
h1: '[lang]/index.astro',
p: 'en',
},
{
url: '/en/index.html',
h1: '[lang]/index.astro',
p: 'en',
},
{
description: 'matches /de/1/2 to [lang]/[...catchall].astro',
url: '/de/1/2',
h1: '[lang]/[...catchall].astro',
p: 'de | 1/2',
},
{
description: 'matches /en/1/2 to [lang]/[...catchall].astro',
url: '/en/1/2',
h1: '[lang]/[...catchall].astro',
p: 'en | 1/2',
},
{
description: 'matches /injected to to-inject.astro',
url: '/injected',
h1: 'to-inject.astro'
},
{
description: 'matches /_injected to to-inject.astro',
url: '/_injected',
h1: 'to-inject.astro'
},
{
description: 'matches /injected-1 to [id].astro',
url: '/injected-1',
h1: '[id].astro',
p: 'injected-1'
},
{
description: 'matches /injected-2 to [id].astro',
url: '/injected-2',
h1: '[id].astro',
p: 'injected-2'
}
];
describe('Routing priority', () => {
before(async () => {
fixture = await loadFixture({
root: './fixtures/routing-priority/',
});
});
function appendForwardSlash(path) {
return path.endsWith('/') ? path : path + '/';
}
describe('Routing priority', () => {
describe('build', () => {
let fixture;
before(async () => {
fixture = await loadFixture({
root: './fixtures/routing-priority/',
});
await fixture.build();
});
it('matches / to index.astro', async () => {
const html = await fixture.readFile('/index.html');
const $ = cheerioLoad(html);
routes.forEach(({ description, url, h1, p }) => {
it(description, async () => {
const html = await fixture.readFile(`${appendForwardSlash(url)}index.html`);
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('index.astro');
});
expect($('h1').text()).to.equal(h1);
it('matches /posts/post-1 to posts/[pid].astro', async () => {
const html = await fixture.readFile('/posts/post-1/index.html');
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('posts/[pid].astro');
expect($('p').text()).to.equal('post-1');
});
it('matches /posts/1/2 to posts/[...slug].astro', async () => {
const html = await fixture.readFile('/posts/1/2/index.html');
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('posts/[...slug].astro');
expect($('p').text()).to.equal('1/2');
});
it('matches /de to de/index.astro', async () => {
const html = await fixture.readFile('/de/index.html');
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('de/index.astro (priority)');
});
it('matches /en to [lang]/index.astro', async () => {
const html = await fixture.readFile('/en/index.html');
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/index.astro');
expect($('p').text()).to.equal('en');
});
it('matches /de/1/2 to [lang]/[...catchall].astro', async () => {
const html = await fixture.readFile('/de/1/2/index.html');
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
expect($('p').text()).to.equal('de | 1/2');
});
it('matches /en/1/2 to [lang]/[...catchall].astro', async () => {
const html = await fixture.readFile('/en/1/2/index.html');
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
expect($('p').text()).to.equal('en | 1/2');
if (p) {
expect($('p').text()).to.equal(p);
}
});
});
});
describe('dev', () => {
let fixture;
let devServer;
before(async () => {
fixture = await loadFixture({
root: './fixtures/routing-priority/',
});
devServer = await fixture.startDevServer();
});
@ -142,91 +142,42 @@ describe('Routing priority', () => {
await devServer.stop();
});
it('matches / to index.astro', async () => {
const html = await fixture.fetch('/').then((res) => res.text());
const $ = cheerioLoad(html);
routes.forEach(({ description, url, h1, p }) => {
// checks URLs as written above
it(description, async () => {
const html = await fixture.fetch(url).then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal(h1);
expect($('h1').text()).to.equal('index.astro');
});
if (p) {
expect($('p').text()).to.equal(p);
}
});
it('matches /posts/post-1 to /posts/[pid].astro', async () => {
const html = await fixture.fetch('/posts/post-1').then((res) => res.text());
const $ = cheerioLoad(html);
// checks with trailing slashes, ex: '/de/' instead of '/de'
it(`${description} (trailing slash)`, async () => {
const html = await fixture.fetch(appendForwardSlash(url)).then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal(h1);
expect($('h1').text()).to.equal('posts/[pid].astro');
expect($('p').text()).to.equal('post-1');
});
if (p) {
expect($('p').text()).to.equal(p);
}
});
it('matches /posts/1/2 to /posts/[...slug].astro', async () => {
const html = await fixture.fetch('/posts/1/2').then((res) => res.text());
const $ = cheerioLoad(html);
// checks with index.html, ex: '/de/index.html' instead of '/de'
it(`${description} (index.html)`, async () => {
const html = await fixture.fetch(`${appendForwardSlash(url)}index.html`).then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal(h1);
expect($('h1').text()).to.equal('posts/[...slug].astro');
expect($('p').text()).to.equal('1/2');
});
it('matches /de to de/index.astro', async () => {
const html = await fixture.fetch('/de').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('de/index.astro (priority)');
expect($('p').text()).to.equal('de');
});
it('matches /de/ to de/index.astro', async () => {
const html = await fixture.fetch('/de/').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('de/index.astro (priority)');
expect($('p').text()).to.equal('de');
});
it('matches /de/index.html to de/index.astro', async () => {
const html = await fixture.fetch('/de/index.html').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('de/index.astro (priority)');
expect($('p').text()).to.equal('de');
});
it('matches /en to [lang]/index.astro', async () => {
const html = await fixture.fetch('/en').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/index.astro');
expect($('p').text()).to.equal('en');
});
it('matches /en/ to [lang]/index.astro', async () => {
const html = await fixture.fetch('/en/').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/index.astro');
expect($('p').text()).to.equal('en');
});
it('matches /en/index.html to de/index.astro', async () => {
const html = await fixture.fetch('/en/index.html').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/index.astro');
expect($('p').text()).to.equal('en');
});
it('matches /de/1/2 to [lang]/[...catchall].astro', async () => {
const html = await fixture.fetch('/de/1/2/index.html').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
expect($('p').text()).to.equal('de | 1/2');
});
it('matches /en/1/2 to [lang]/[...catchall].astro', async () => {
const html = await fixture.fetch('/en/1/2/index.html').then((res) => res.text());
const $ = cheerioLoad(html);
expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
expect($('p').text()).to.equal('en | 1/2');
if (p) {
expect($('p').text()).to.equal(p);
}
});
});
});
});