Fix subpath support regressions (#2330)

* Fix subpath support regressions

* Adds a changeset

* Update tests to reflect relative URL change

* Pick a different port and hopefully windows works

* Remove bad lint warning

* Better handling of relative paths

* or

* Fixes use with pageUrlFormat

* Update the pageDirectoryUrl test
This commit is contained in:
Matthew Phillips 2022-01-06 16:32:24 -05:00 committed by GitHub
parent 8b58ede2cc
commit 71ca09125a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 566 additions and 22 deletions

View file

@ -0,0 +1,5 @@
---
'astro': patch
---
Fixes subpath support in `astro preview`

View file

@ -12,6 +12,7 @@ module.exports = {
'@typescript-eslint/no-unused-vars': 'off',
'@typescript-eslint/no-use-before-define': 'off',
'@typescript-eslint/no-var-requires': 'off',
'@typescript-eslint/no-this-alias': 'off',
'no-console': 'warn',
'no-shadow': 'error',
'prefer-const': 'off',

View file

@ -0,0 +1,37 @@
export function appendForwardSlash(path: string) {
return path.endsWith('/') ? path : path + '/';
}
export function prependForwardSlash(path: string) {
return path[0] === '/' ? path : '/' + path;
}
export function removeEndingForwardSlash(path: string) {
return path.endsWith('/') ? path.slice(0, path.length - 1) : path;
}
export function startsWithDotDotSlash(path: string) {
const c1 = path[0];
const c2 = path[1];
const c3 = path[2];
return c1 === '.' && c2 === '.' && c3 === '/';
}
export function startsWithDotSlash(path: string) {
const c1 = path[0];
const c2 = path[1];
return c1 === '.' && c2 === '/';
}
export function isRelativePath(path: string) {
return startsWithDotDotSlash(path) || startsWithDotSlash(path);
}
export function prependDotSlash(path: string) {
if(isRelativePath(path)) {
return path;
}
return './' + path;
}

View file

@ -7,19 +7,33 @@ import send from 'send';
import { fileURLToPath } from 'url';
import * as msg from '../dev/messages.js';
import { error, info } from '../logger.js';
import { subpathNotUsedTemplate } from '../dev/template/4xx.js';
import { subpathNotUsedTemplate, default as template } from '../dev/template/4xx.js';
import { prependForwardSlash } from '../path.js';
import * as npath from 'path';
import * as fs from 'fs';
interface PreviewOptions {
logging: LogOptions;
}
interface PreviewServer {
export interface PreviewServer {
hostname: string;
port: number;
server: http.Server;
stop(): Promise<void>;
}
type SendStreamWithPath = send.SendStream & { path: string };
function removeBase(base: string, pathname: string) {
if(base === pathname) {
return '/';
}
let requrl = pathname.substr(base.length);
return prependForwardSlash(requrl);
}
/** The primary dev action */
export default async function preview(config: AstroConfig, { logging }: PreviewOptions): Promise<PreviewServer> {
const startServerTime = performance.now();
@ -33,9 +47,73 @@ export default async function preview(config: AstroConfig, { logging }: PreviewO
return;
}
send(req, req.url!.substr(base.length - 1), {
root: fileURLToPath(config.dist),
}).pipe(res);
switch(config.devOptions.trailingSlash) {
case 'always': {
if(!req.url?.endsWith('/')) {
res.statusCode = 404;
res.end(template({
title: 'Not found',
tabTitle: 'Not found',
pathname: req.url!,
}));
return;
}
break;
}
case 'never': {
if(req.url?.endsWith('/')) {
res.statusCode = 404;
res.end(template({
title: 'Not found',
tabTitle: 'Not found',
pathname: req.url!,
}));
return;
}
break;
}
case 'ignore': {
break;
}
}
let sendpath = removeBase(base, req.url!);
const sendOptions: send.SendOptions = {
root: fileURLToPath(config.dist)
};
if(config.buildOptions.pageUrlFormat === 'file' && !sendpath.endsWith('.html')) {
sendOptions.index = false;
const parts = sendpath.split('/');
let lastPart = parts.pop();
switch(config.devOptions.trailingSlash) {
case 'always': {
lastPart = parts.pop();
break;
}
case 'never': {
// lastPart is the actually last part like `page`
break;
}
case 'ignore': {
// this could end in slash, so resolve either way
if(lastPart === '') {
lastPart = parts.pop();
}
break;
}
}
const part = lastPart || 'index';
sendpath = npath.sep + npath.join(...parts, `${part}.html`);
}
send(req, sendpath, sendOptions)
.once('directory', function(this: SendStreamWithPath, _res, path) {
if(config.buildOptions.pageUrlFormat === 'directory' && !path.endsWith('index.html')) {
return this.sendIndex(path);
} else {
this.error(404);
}
})
.pipe(res);
});
let { hostname, port } = config.devOptions;

View file

@ -14,6 +14,7 @@ import { findAssets, findExternalScripts, findInlineScripts, findInlineStyles, g
import { isBuildableImage, isBuildableLink, isHoistedScript, isInSrcDirectory, hasSrcSet } from './util.js';
import { render as ssrRender } from '../core/ssr/index.js';
import { getAstroStyleId, getAstroPageStyleId } from '../vite-plugin-build-css/index.js';
import { prependDotSlash, removeEndingForwardSlash } from '../core/path.js';
// This package isn't real ESM, so have to coerce it
const matchSrcset: typeof srcsetParse = (srcsetParse as any).default;
@ -36,6 +37,11 @@ interface PluginOptions {
viteServer: ViteDevServer;
}
function relativePath(from: string, to: string): string {
const rel = npath.posix.relative(from, to);
return prependDotSlash(rel);
}
export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
const { astroConfig, internals, logging, origin, allPages, routeCache, viteServer, pageNames } = options;
@ -74,7 +80,9 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
}
for (const pathname of pageData.paths) {
pageNames.push(pathname.replace(/\/?$/, '/index.html').replace(/^\//, ''));
const pathrepl = astroConfig.buildOptions.pageUrlFormat === 'directory' ?
'/index.html' : pathname === '/' ? 'index.html' : '.html';
pageNames.push(pathname.replace(/\/?$/, pathrepl).replace(/^\//, ''));
const id = ASTRO_PAGE_PREFIX + pathname;
const html = await ssrRender(renderers, mod, {
astroConfig,
@ -309,7 +317,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
const lastNode = ref;
for (const referenceId of referenceIds) {
const chunkFileName = this.getFileName(referenceId);
const relPath = npath.posix.relative(pathname, '/' + chunkFileName);
const relPath = relativePath(pathname, '/' + chunkFileName);
// This prevents added links more than once per type.
const key = pathname + relPath + attrs.rel || 'stylesheet';
@ -350,7 +358,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
if (getAttribute(script, 'astro-script') && typeof pageAssetId === 'string') {
if (!pageBundleAdded) {
pageBundleAdded = true;
const relPath = npath.posix.relative(pathname, bundlePath);
const relPath = relativePath(pathname, bundlePath);
insertBefore(
script.parentNode,
createScript({
@ -369,7 +377,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
if (getAttribute(script, 'astro-script') && typeof pageAssetId === 'string') {
if (!pageBundleAdded) {
pageBundleAdded = true;
const relPath = npath.posix.relative(pathname, bundlePath);
const relPath = relativePath(pathname, bundlePath);
insertBefore(
script.parentNode,
createScript({
@ -389,7 +397,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
// On windows the facadeId doesn't start with / but does not Unix :/
if (src && (facadeIdMap.has(src) || facadeIdMap.has(src.substr(1)))) {
const assetRootPath = '/' + (facadeIdMap.get(src) || facadeIdMap.get(src.substr(1)));
const relPath = npath.posix.relative(pathname, assetRootPath);
const relPath = relativePath(pathname, assetRootPath);
const attrs = getAttributes(script);
insertBefore(
script.parentNode,
@ -434,7 +442,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
const referenceId = assetIdMap.get(src);
if (referenceId) {
const fileName = this.getFileName(referenceId);
const relPath = npath.posix.relative(pathname, '/' + fileName);
const relPath = relativePath(pathname, '/' + fileName);
setAttribute(node, 'src', relPath);
}
}
@ -448,7 +456,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
if (assetIdMap.has(url)) {
const referenceId = assetIdMap.get(url)!;
const fileName = this.getFileName(referenceId);
const relPath = npath.posix.relative(pathname, '/' + fileName);
const relPath = relativePath(pathname, '/' + fileName);
changedSrcset = changedSrcset.replace(url, relPath);
}
}
@ -476,8 +484,8 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
// Output directly to 404.html rather than 400/index.html
// Supports any other status codes, too
if (name.match(STATUS_CODE_RE)) {
outPath = npath.posix.join(`${name}.html`);
if (name.match(STATUS_CODE_RE) || astroConfig.buildOptions.pageUrlFormat === 'file') {
outPath = `${removeEndingForwardSlash(name || 'index')}.html`;
} else {
outPath = npath.posix.join(name, 'index.html');
}

View file

@ -31,7 +31,7 @@ describe('CSS', function () {
// get bundled CSS (will be hashed, hence DOM query)
const html = await fixture.readFile('/index.html');
$ = cheerio.load(html);
const bundledCSSHREF = $('link[rel=stylesheet][href^=assets/]').attr('href');
const bundledCSSHREF = $('link[rel=stylesheet][href^=./assets/]').attr('href');
bundledCSS = await fixture.readFile(bundledCSSHREF.replace(/^\/?/, '/'));
});

View file

@ -5,7 +5,7 @@ import { loadFixture } from './test-utils.js';
// note: the hashes should be deterministic, but updating the file contents will change hashes
// be careful not to test that the HTML simply contains CSS, because it always will! filename and quanity matter here (bundling).
const EXPECTED_CSS = {
'/index.html': ['assets/index', 'assets/typography'], // dont match hashes, which change based on content
'/index.html': ['./assets/index', './assets/typography'], // dont match hashes, which change based on content
'/one/index.html': ['../assets/one'],
'/two/index.html': ['../assets/two', '../assets/typography'],
'/preload/index.html': ['../assets/preload'],

View file

@ -15,8 +15,8 @@ describe('pageUrlFormat', () => {
});
it('outputs', async () => {
expect(await fixture.readFile('/client/index.html')).to.be.ok;
expect(await fixture.readFile('/nested-md/index.html')).to.be.ok;
expect(await fixture.readFile('/nested-astro/index.html')).to.be.ok;
expect(await fixture.readFile('/client.html')).to.be.ok;
expect(await fixture.readFile('/nested-md.html')).to.be.ok;
expect(await fixture.readFile('/nested-astro.html')).to.be.ok;
});
});

View file

@ -3,4 +3,4 @@ export default {
buildOptions: {
site: 'http://example.com/blog'
}
}
}

View file

@ -17,7 +17,7 @@ before(async () => {
// get bundled CSS (will be hashed, hence DOM query)
const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);
const bundledCSSHREF = $('link[rel=stylesheet][href^=assets/]').attr('href');
const bundledCSSHREF = $('link[rel=stylesheet][href^=./assets/]').attr('href');
bundledCSS = await fixture.readFile(bundledCSSHREF.replace(/^\/?/, '/'));
});

View file

@ -0,0 +1,413 @@
import { expect } from 'chai';
import { loadFixture } from './test-utils.js';
describe('Preview Routing', () => {
describe('pageUrlFormat: directory', () => {
describe('Subpath without trailing slash and trailingSlash: never', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;
before(async () => {
fixture = await loadFixture({
projectRoot: './fixtures/with-subpath-no-trailing-slash/',
devOptions: {
trailingSlash: 'never',
port: 4000
}
});
await fixture.build();
previewServer = await fixture.preview();
});
after(async () => {
previewServer && (await previewServer.stop());
});
it('404 when loading /', async () => {
const response = await fixture.fetch('/');
expect(response.status).to.equal(404);
});
it('404 when loading subpath root with trailing slash', async () => {
const response = await fixture.fetch('/blog/');
expect(response.status).to.equal(404);
});
it('200 when loading subpath root without trailing slash', async () => {
const response = await fixture.fetch('/blog');
expect(response.status).to.equal(200);
expect(response.redirected).to.equal(false);
});
it('404 when loading another page with subpath used', async () => {
const response = await fixture.fetch('/blog/another/');
expect(response.status).to.equal(404);
});
it('200 when loading dynamic route', async () => {
const response = await fixture.fetch('/blog/1');
expect(response.status).to.equal(200);
});
it('404 when loading invalid dynamic route', async () => {
const response = await fixture.fetch('/blog/2');
expect(response.status).to.equal(404);
});
});
describe('Subpath without trailing slash and trailingSlash: always', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;
before(async () => {
fixture = await loadFixture({
projectRoot: './fixtures/with-subpath-no-trailing-slash/',
devOptions: {
trailingSlash: 'always',
port: 4001
}
});
await fixture.build();
previewServer = await fixture.preview();
});
after(async () => {
previewServer && (await previewServer.stop());
});
it('404 when loading /', async () => {
const response = await fixture.fetch('/');
expect(response.status).to.equal(404);
});
it('200 when loading subpath root with trailing slash', async () => {
const response = await fixture.fetch('/blog/');
expect(response.status).to.equal(200);
});
it('404 when loading subpath root without trailing slash', async () => {
const response = await fixture.fetch('/blog');
expect(response.status).to.equal(404);
expect(response.redirected).to.equal(false);
});
it('200 when loading another page with subpath used', async () => {
const response = await fixture.fetch('/blog/another/');
expect(response.status).to.equal(200);
});
it('404 when loading another page with subpath not used', async () => {
const response = await fixture.fetch('/blog/another');
expect(response.status).to.equal(404);
});
it('200 when loading dynamic route', async () => {
const response = await fixture.fetch('/blog/1/');
expect(response.status).to.equal(200);
});
it('404 when loading invalid dynamic route', async () => {
const response = await fixture.fetch('/blog/2/');
expect(response.status).to.equal(404);
});
});
describe('Subpath without trailing slash and trailingSlash: ignore', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;
before(async () => {
fixture = await loadFixture({
projectRoot: './fixtures/with-subpath-no-trailing-slash/',
devOptions: {
trailingSlash: 'ignore',
port: 4002
}
});
await fixture.build();
previewServer = await fixture.preview();
});
after(async () => {
previewServer && (await previewServer.stop());
});
it('404 when loading /', async () => {
const response = await fixture.fetch('/');
expect(response.status).to.equal(404);
});
it('200 when loading subpath root with trailing slash', async () => {
const response = await fixture.fetch('/blog/');
expect(response.status).to.equal(200);
});
it('200 when loading subpath root without trailing slash', async () => {
const response = await fixture.fetch('/blog');
expect(response.status).to.equal(200);
expect(response.redirected).to.equal(false);
});
it('200 when loading another page with subpath used', async () => {
const response = await fixture.fetch('/blog/another/');
expect(response.status).to.equal(200);
});
it('200 when loading another page with subpath not used', async () => {
const response = await fixture.fetch('/blog/another');
expect(response.status).to.equal(200);
});
it('200 when loading dynamic route', async () => {
const response = await fixture.fetch('/blog/1/');
expect(response.status).to.equal(200);
});
it('404 when loading invalid dynamic route', async () => {
const response = await fixture.fetch('/blog/2/');
expect(response.status).to.equal(404);
});
});
});
describe('pageUrlFormat: file', () => {
describe('Subpath without trailing slash and trailingSlash: never', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;
before(async () => {
fixture = await loadFixture({
projectRoot: './fixtures/with-subpath-no-trailing-slash/',
buildOptions: {
pageUrlFormat: 'file'
},
devOptions: {
trailingSlash: 'never',
port: 4003
}
});
await fixture.build();
previewServer = await fixture.preview();
});
after(async () => {
previewServer && (await previewServer.stop());
});
it('404 when loading /', async () => {
const response = await fixture.fetch('/');
expect(response.status).to.equal(404);
});
it('404 when loading subpath root with trailing slash', async () => {
const response = await fixture.fetch('/blog/');
expect(response.status).to.equal(404);
});
it('200 when loading subpath root without trailing slash', async () => {
const response = await fixture.fetch('/blog');
expect(response.status).to.equal(200);
expect(response.redirected).to.equal(false);
});
it('404 when loading another page with subpath used', async () => {
const response = await fixture.fetch('/blog/another/');
expect(response.status).to.equal(404);
});
it('200 when loading dynamic route', async () => {
const response = await fixture.fetch('/blog/1');
expect(response.status).to.equal(200);
});
it('404 when loading invalid dynamic route', async () => {
const response = await fixture.fetch('/blog/2');
expect(response.status).to.equal(404);
});
});
describe('Subpath without trailing slash and trailingSlash: always', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;
before(async () => {
fixture = await loadFixture({
projectRoot: './fixtures/with-subpath-no-trailing-slash/',
buildOptions: {
pageUrlFormat: 'file'
},
devOptions: {
trailingSlash: 'always',
port: 4004
}
});
await fixture.build();
previewServer = await fixture.preview();
});
after(async () => {
previewServer && (await previewServer.stop());
});
it('404 when loading /', async () => {
const response = await fixture.fetch('/');
expect(response.status).to.equal(404);
});
it('200 when loading subpath root with trailing slash', async () => {
const response = await fixture.fetch('/blog/');
expect(response.status).to.equal(200);
});
it('404 when loading subpath root without trailing slash', async () => {
const response = await fixture.fetch('/blog');
expect(response.status).to.equal(404);
expect(response.redirected).to.equal(false);
});
it('200 when loading another page with subpath used', async () => {
const response = await fixture.fetch('/blog/another/');
expect(response.status).to.equal(200);
});
it('404 when loading another page with subpath not used', async () => {
const response = await fixture.fetch('/blog/another');
expect(response.status).to.equal(404);
});
it('200 when loading dynamic route', async () => {
const response = await fixture.fetch('/blog/1/');
expect(response.status).to.equal(200);
});
it('404 when loading invalid dynamic route', async () => {
const response = await fixture.fetch('/blog/2/');
expect(response.status).to.equal(404);
});
});
describe('Subpath without trailing slash and trailingSlash: ignore', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;
before(async () => {
fixture = await loadFixture({
projectRoot: './fixtures/with-subpath-no-trailing-slash/',
buildOptions: {
pageUrlFormat: 'file'
},
devOptions: {
trailingSlash: 'ignore',
port: 4005
}
});
await fixture.build();
previewServer = await fixture.preview();
});
after(async () => {
previewServer && (await previewServer.stop());
});
it('404 when loading /', async () => {
const response = await fixture.fetch('/');
expect(response.status).to.equal(404);
});
it('200 when loading subpath root with trailing slash', async () => {
const response = await fixture.fetch('/blog/');
expect(response.status).to.equal(200);
});
it('200 when loading subpath root without trailing slash', async () => {
const response = await fixture.fetch('/blog');
expect(response.status).to.equal(200);
expect(response.redirected).to.equal(false);
});
it('200 when loading another page with subpath used', async () => {
const response = await fixture.fetch('/blog/another/');
expect(response.status).to.equal(200);
});
it('200 when loading another page with subpath not used', async () => {
const response = await fixture.fetch('/blog/another');
expect(response.status).to.equal(200);
});
it('200 when loading dynamic route', async () => {
const response = await fixture.fetch('/blog/1/');
expect(response.status).to.equal(200);
});
it('404 when loading invalid dynamic route', async () => {
const response = await fixture.fetch('/blog/2/');
expect(response.status).to.equal(404);
});
});
describe('Exact file path', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;
before(async () => {
fixture = await loadFixture({
projectRoot: './fixtures/with-subpath-no-trailing-slash/',
buildOptions: {
pageUrlFormat: 'file'
},
devOptions: {
trailingSlash: 'ignore',
port: 4006
}
});
await fixture.build();
previewServer = await fixture.preview();
});
after(async () => {
previewServer && (await previewServer.stop());
});
it('404 when loading /', async () => {
const response = await fixture.fetch('/');
expect(response.status).to.equal(404);
});
it('200 when loading subpath with index.html', async () => {
const response = await fixture.fetch('/blog/index.html');
expect(response.status).to.equal(200);
});
it('200 when loading another page with subpath used', async () => {
const response = await fixture.fetch('/blog/another.html');
expect(response.status).to.equal(200);
});
it('200 when loading dynamic route', async () => {
const response = await fixture.fetch('/blog/1.html');
expect(response.status).to.equal(200);
});
it('404 when loading invalid dynamic route', async () => {
const response = await fixture.fetch('/blog/2.html');
expect(response.status).to.equal(404);
});
});
});
});

View file

@ -10,7 +10,8 @@ import os from 'os';
/**
* @typedef {import('node-fetch').Response} Response
* @typedef {import('../src/core/dev/index').DevServer} DevServer
* @typedef {import('../src/@types/astro').AstroConfig AstroConfig}
* @typedef {import('../src/@types/astro').AstroConfig} AstroConfig
* @typedef {import('../src/core/preview/index').PreviewServer} PreviewServer
*
*
* @typedef {Object} Fixture
@ -19,6 +20,7 @@ import os from 'os';
* @property {(path: string) => Promise<string>} readFile
* @property {(path: string) => Promise<string[]>} readdir
* @property {() => Promise<DevServer>} startDevServer
* @property {() => Promise<PreviewServer>} preview
*/
/**