Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix injected route edge case #7399

Merged
merged 6 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/large-ants-tap.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fix edge case where injected routes would cause builds to fail in a PNPM workspace
7 changes: 6 additions & 1 deletion packages/astro/src/core/build/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
type BuildInternals,
} from '../../core/build/internal.js';
import {
isRelativePath,
prependForwardSlash,
removeLeadingForwardSlash,
removeTrailingForwardSlash,
Expand Down Expand Up @@ -252,7 +253,11 @@ async function generatePage(
};

const icon = pageData.route.type === 'page' ? green('▶') : magenta('λ');
info(opts.logging, null, `${icon} ${pageData.route.component}`);
if (isRelativePath(pageData.route.component)) {
info(opts.logging, null, `${icon} ${pageData.route.route}`);
} else {
info(opts.logging, null, `${icon} ${pageData.route.component}`);
}

// Get paths for the route, calling getStaticPaths if needed.
const paths = await getPathsForRoute(pageData, pageModule, opts, builtPaths);
Expand Down
38 changes: 19 additions & 19 deletions packages/astro/src/core/build/static-build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
} from './plugins/plugin-pages.js';
import { RESOLVED_RENDERERS_MODULE_ID } from './plugins/plugin-renderers.js';
import { SSR_VIRTUAL_MODULE_ID } from './plugins/plugin-ssr.js';
import type { PageBuildData, StaticBuildOptions } from './types';
import type { AllPagesData, PageBuildData, StaticBuildOptions } from './types';
import { getTimeStat } from './util.js';

export async function viteBuild(opts: StaticBuildOptions) {
Expand Down Expand Up @@ -143,7 +143,7 @@ async function ssrBuild(
input: Set<string>,
container: AstroBuildPluginContainer
) {
const { settings, viteConfig } = opts;
const { allPages, settings, viteConfig } = opts;
const ssr = isServerLikeOutput(settings.config);
const out = ssr ? opts.buildConfig.server : getOutDirWithinCwd(settings.config.outDir);

Expand Down Expand Up @@ -175,7 +175,7 @@ async function ssrBuild(
...viteConfig.build?.rollupOptions?.output,
entryFileNames(chunkInfo) {
if (chunkInfo.facadeModuleId?.startsWith(ASTRO_PAGE_RESOLVED_MODULE_ID)) {
return makeAstroPageEntryPointFileName(chunkInfo.facadeModuleId);
return makeAstroPageEntryPointFileName(chunkInfo.facadeModuleId, allPages);
} else if (
// checks if the path of the module we have middleware, e.g. middleware.js / middleware/index.js
chunkInfo.moduleIds.find((m) => m.includes('middleware')) !== undefined &&
Expand Down Expand Up @@ -418,27 +418,27 @@ async function ssrMoveAssets(opts: StaticBuildOptions) {
}

/**
* This function takes as input the virtual module name of an astro page and transform
* to generate an `.mjs` file:
* This function takes the virtual module name of any page entrypoint and
* transforms it to generate a final `.mjs` output file.
*
* Input: `@astro-page:src/pages/index@_@astro`
*
* Output: `pages/index.astro.mjs`
* Input: `@astro-page:../node_modules/my-dep/injected@_@astro`
* Output: `pages/injected.mjs`
*
* 1. We remove the module id prefix, `@astro-page:`
* 2. We remove `src/`
* 3. We replace square brackets with underscore, for example `[slug]`
* 4. At last, we replace the extension pattern with a simple dot
* 5. We append the `.mjs` string, so the file will always be a JS file
* 1. We clean the `facadeModuleId` by removing the `@astro-page:` prefix and `@_@` suffix
* 2. We find the matching route pattern in the manifest (or fallback to the cleaned module id)
* 3. We replace square brackets with underscore (`[slug]` => `_slug_`) and `...` with `` (`[...slug]` => `_---slug_`).
* 4. We append the `.mjs` extension, so the file will always be an ESM module
*
* @param facadeModuleId
* @param facadeModuleId string
* @param pages AllPagesData
*/
function makeAstroPageEntryPointFileName(facadeModuleId: string) {
return `${facadeModuleId
function makeAstroPageEntryPointFileName(facadeModuleId: string, pages: AllPagesData) {
const pageModuleId = facadeModuleId
.replace(ASTRO_PAGE_RESOLVED_MODULE_ID, '')
.replace('src/', '')
.replaceAll('[', '_')
.replaceAll(']', '_')
// this must be last
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
.replace(ASTRO_PAGE_EXTENSION_POST_PATTERN, '.')}.mjs`;
.replace(ASTRO_PAGE_EXTENSION_POST_PATTERN, '.');
let name = pages[pageModuleId]?.route?.route ?? pageModuleId;
if (name.endsWith('/')) name += 'index';
return `pages${name.replaceAll('[', '_').replaceAll(']', '_').replaceAll('...', '---')}.mjs`;
}
22 changes: 22 additions & 0 deletions packages/astro/test/custom-404-injected-from-dep.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { expect } from 'chai';
import { loadFixture } from './test-utils.js';

describe('Custom 404 with injectRoute from dependency', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a specific test for #7397! The injected route will be matched to a relative path in ./node_modules but the build should still succeed. This test verifies that this is the case.

describe('build', () => {
/** @type {import('./test-utils.js').Fixture} */
let fixture;

before(async () => {
fixture = await loadFixture({
root: './fixtures/custom-404-injected-from-dep/',
site: 'http://example.com',
});
await fixture.build();
});

it('Build succeeds', async () => {
const html = await fixture.readFile('/404.html');
expect(html).to.contain('<!DOCTYPE html>');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { defineConfig } from 'astro/config';

// https://astro.build/config
export default defineConfig({
integrations: [
{
name: '404-integration',
hooks: {
'astro:config:setup': ({ injectRoute }) => {
injectRoute({
pattern: '404',
entryPoint: '@test/custom-404-pkg/404.astro',
});
},
},
},
],
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "@test/custom-404-injected-from-dep",
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*",
"@test/custom-404-pkg": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
---

<html lang="en">
<head>
<title>Custom 404</title>
</head>
<body>
<h1>Home</h1>
</body>
</html>
13 changes: 13 additions & 0 deletions packages/astro/test/fixtures/custom-404-pkg/404.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
const canonicalURL = new URL(Astro.url.pathname, Astro.site);
---

<html lang="en">
<head>
<title>Not Found - Custom 404</title>
</head>
<body>
<h1>Page not found</h1>
<p>{canonicalURL.pathname}</p>
</body>
</html>
8 changes: 8 additions & 0 deletions packages/astro/test/fixtures/custom-404-pkg/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@test/custom-404-pkg",
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*"
}
}
17 changes: 16 additions & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.