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

🗂 Support url slugs with multiple path segments #489

Merged
merged 8 commits into from
Oct 24, 2024
Merged
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
8 changes: 8 additions & 0 deletions .changeset/early-plums-double.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@myst-theme/common': patch
'@myst-theme/article': patch
'@myst-theme/site': patch
'@myst-theme/book': patch
---

Support url slugs with multiple path segments
88 changes: 44 additions & 44 deletions package-lock.json

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

6 changes: 3 additions & 3 deletions packages/common/package.json
Original file line number Diff line number Diff line change
@@ -19,9 +19,9 @@
"build": "npm-run-all -l clean -p build:esm"
},
"dependencies": {
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-spec-ext": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"myst-spec-ext": "^1.7.3",
"nbtx": "^0.2.3",
"unist-util-select": "^4.0.3"
}
4 changes: 3 additions & 1 deletion packages/common/src/utils.ts
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ import type { SiteManifest } from 'myst-config';
import { selectAll } from 'unist-util-select';
import type { Image as ImageSpec, Link as LinkSpec } from 'myst-spec';
import type { FooterLinks, Heading, NavigationLink, PageLoader } from './types.js';
import { slugToUrl } from 'myst-common';

type Image = ImageSpec & { urlOptimized?: string };
type Link = LinkSpec & { static?: boolean };
@@ -40,9 +41,10 @@ export function getProjectHeadings(
},
...project.pages.map((p) => {
if (!('slug' in p)) return p;
const slug = slugToUrl(p.slug);
return {
...p,
path: projectSlug && project.slug ? `/${project.slug}/${p.slug}` : `/${p.slug}`,
path: projectSlug && project.slug ? `/${project.slug}/${slug}` : `/${slug}`,
};
}),
];
8 changes: 4 additions & 4 deletions packages/jupyter/package.json
Original file line number Diff line number Diff line change
@@ -30,11 +30,11 @@
"buffer": "^6.0.3",
"classnames": "^2.5.1",
"jupyterlab-plotly": "^5.24.0",
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-frontmatter": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"myst-frontmatter": "^1.7.3",
"myst-spec": "^0.0.5",
"myst-spec-ext": "^1.7.2",
"myst-spec-ext": "^1.7.3",
"myst-to-react": "^0.13.2",
"nanoid": "^4.0.2",
"nbtx": "^0.2.3",
8 changes: 4 additions & 4 deletions packages/myst-demo/package.json
Original file line number Diff line number Diff line change
@@ -23,23 +23,23 @@
"@heroicons/react": "^2.0.18",
"classnames": "^2.3.2",
"js-yaml": "^4.1.0",
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"myst-directives": "^1.5.7",
"myst-ext-card": "^1.0.9",
"myst-ext-exercise": "^1.0.8",
"myst-ext-grid": "^1.0.8",
"myst-ext-proof": "^1.0.11",
"myst-ext-tabs": "^1.0.8",
"myst-frontmatter": "^1.7.2",
"myst-frontmatter": "^1.7.3",
"myst-parser": "^1.5.7",
"myst-spec": "^0.0.5",
"myst-to-docx": "^1.0.12",
"myst-to-html": "^1.5.7",
"myst-to-jats": "^1.0.30",
"myst-to-react": "^0.13.2",
"myst-to-tex": "^1.0.38",
"myst-to-typst": "^0.0.24",
"myst-to-typst": "^0.0.25",
"myst-transforms": "^1.3.26",
"unified": "^10.1.2",
"unist-util-remove": "^4.0.0",
4 changes: 2 additions & 2 deletions packages/myst-to-react/package.json
Original file line number Diff line number Diff line change
@@ -26,8 +26,8 @@
"@radix-ui/react-hover-card": "^1.0.6",
"buffer": "^6.0.3",
"classnames": "^2.3.2",
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"myst-spec": "^0.0.5",
"nanoid": "^4.0.2",
"react-syntax-highlighter": "15.5.0",
6 changes: 3 additions & 3 deletions packages/providers/package.json
Original file line number Diff line number Diff line change
@@ -27,9 +27,9 @@
"peerDependencies": {
"@types/react": "^16.8 || ^17.0 || ^18.0",
"@types/react-dom": "^16.8 || ^17.0 || ^18.0",
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-frontmatter": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"myst-frontmatter": "^1.7.3",
"react": "^16.8 || ^17.0 || ^18.0",
"react-dom": "^16.8 || ^17.0 || ^18.0"
},
6 changes: 3 additions & 3 deletions packages/site/package.json
Original file line number Diff line number Diff line change
@@ -35,10 +35,10 @@
"@radix-ui/react-visually-hidden": "^1.1.0",
"classnames": "^2.3.2",
"lodash.throttle": "^4.1.1",
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"myst-demo": "^0.13.2",
"myst-spec-ext": "^1.7.2",
"myst-spec-ext": "^1.7.3",
"myst-to-react": "^0.13.2",
"nbtx": "^0.2.3",
"node-cache": "^5.1.2",
2 changes: 1 addition & 1 deletion packages/site/src/components/Navigation/Navigation.tsx
Original file line number Diff line number Diff line change
@@ -77,7 +77,7 @@ export const ConfigurablePrimaryNavigation = ({

if (children)
console.warn(
`Including children in Navigation can break keyboard accessbility and is deprecated. Please move children to the page component.`,
`Including children in Navigation can break keyboard accessibility and is deprecated. Please move children to the page component.`,
);

// the logic on the following line looks wrong, this will return `null` or `<></>`
10 changes: 8 additions & 2 deletions packages/site/src/components/Navigation/TableOfContentsItems.tsx
Original file line number Diff line number Diff line change
@@ -35,10 +35,16 @@ function nestToc(toc: Heading[]): NestedHeading[] {
return items;
}

function pathnameMatchesHeading(pathname: string, heading: Heading, baseurl?: string) {
const headingPath = withBaseurl(heading.path, baseurl);
if (pathname && headingPath === `${pathname}/index`) return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a few small changes in here that allow index pages to be accessible at the folder-path url.

For example, the file folder1/folder2/index.md is available at /folder1/folder2 (and /folder1/folder2/index redirects back to /folder1/folder2).

return headingPath === pathname;
}

function childrenOpen(headings: NestedHeading[], pathname: string, baseurl?: string): string[] {
return headings
.map((heading) => {
if (withBaseurl(heading.path, baseurl) === pathname) return [heading.id];
if (pathnameMatchesHeading(pathname, heading, baseurl)) return [heading.id];
const open = childrenOpen(heading.children, pathname, baseurl);
if (open.length === 0) return [];
return [heading.id, ...open];
@@ -110,7 +116,7 @@ const NestedToc = ({ heading }: { heading: NestedHeading }) => {
useEffect(() => {
if (nav.state === 'idle') setOpen(startOpen);
}, [nav.state]);
const exact = pathname === withBaseurl(heading.path, baseurl);
const exact = pathnameMatchesHeading(pathname, heading, baseurl);
if (!heading.children || heading.children.length === 0) {
return (
<LinkItem
3 changes: 2 additions & 1 deletion packages/site/src/seo/sitemap.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { slugToUrl } from 'myst-common';
import type { SiteManifest } from 'myst-config';

type ManifestProjectItem = Required<SiteManifest>['projects'][0]['pages'][0];
@@ -148,7 +149,7 @@ export function getSiteSlugs(
const projectSlug = project.slug ? `/${project.slug}` : '';
const pages = project.pages
.filter((page): page is ManifestProjectItem => 'slug' in page)
.map((page) => `${baseurl}${projectSlug}/${page.slug}`);
.map((page) => `${baseurl}${projectSlug}/${slugToUrl(page.slug)}`);
if (opts?.excludeIndex) return [...pages];
return [
opts?.explicitIndex
11 changes: 6 additions & 5 deletions themes/article/app/routes/$.tsx
Original file line number Diff line number Diff line change
@@ -47,14 +47,15 @@ export const meta: V2_MetaFunction<typeof loader> = ({ data, matches, location }
export const links: LinksFunction = () => [KatexCSS];

export const loader: LoaderFunction = async ({ params, request }) => {
const [first, second] = new URL(request.url).pathname.slice(1).split('/');
const projectName = second ? first : undefined;
const slug = second || first;
const [first, ...rest] = new URL(request.url).pathname.slice(1).split('/');
const config = await getConfig();
const project = getProject(config, projectName ?? slug);
const project = getProject(config, first);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I learned about some strangeness in the article theme while making these changes: the article theme does not work correctly with project slugs. I think it is ok if project slugs just do not work with article theme, but we should make that more clear. For example, here we should not even try to load the project based on the URL; it should always just be the default single project.

However, for this PR, I tried as best as I could to keep parity with the old functionality.

const projectName = project?.slug === first ? first : undefined;
const slugParts = projectName ? rest : [first, ...rest];
const slug = slugParts.length ? slugParts.join('.') : undefined;
const flat = isFlatSite(config);
const page = await getPage(request, {
project: flat ? projectName : projectName ?? slug,
project: flat ? projectName : (projectName ?? slug),
slug: flat ? slug : projectName ? slug : undefined,
redirect: process.env.MODE === 'static' ? false : true,
});
Original file line number Diff line number Diff line change
@@ -13,23 +13,27 @@ function api404(message = 'No API route found at this URL') {
}

export const loader: LoaderFunction = async ({ request, params }) => {
const { slug } = params;
const [first, ...rest] = new URL(request.url).pathname
.slice(1)
.replace(/\.json$/, '')
.split('/');
// Handle /myst.xref.json as slug
if (slug === 'myst.xref') {
if (rest.length === 0 && first === 'myst.xref') {
const xref = await getMystXrefJson();
if (!xref) {
return json({ message: 'myst.xref.json not found', status: 404 }, { status: 404 });
}
return json(xref);
}
// Handle /myst.search.json as slug
else if (slug === 'myst.search') {
else if (rest.length === 0 && first === 'myst.search') {
const search = await getMystSearchJson();
if (!search) {
return json({ message: 'myst.search.json not found', status: 404 }, { status: 404 });
}
return json(search);
}
const slug = [first, ...rest].join('.');
const data = await getPage(request, { slug }).catch(() => null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Look at this for example - on this route, we do not mention anything about project. The other route should probably be the same way.)

if (!data) return api404('No page found at this URL.');
return json(data, {
17 changes: 13 additions & 4 deletions themes/article/app/utils/loaders.server.ts
Original file line number Diff line number Diff line change
@@ -62,11 +62,20 @@ export async function getPage(
const project = getProject(config, projectName);
if (!project) throw responseNoArticle();
if (opts.slug === project.index && opts.redirect) {
return redirect(projectName ? `/${projectName}` : '/');
throw redirect(projectName ? `/${projectName}` : '/');
}
if (opts.slug?.endsWith('.index') && opts.redirect) {
const newSlug = opts.slug.slice(0, -6);
throw redirect(projectName ? `/${projectName}/${newSlug}` : `/${newSlug}`);
}
let slug = opts.loadIndexPage || opts.slug == null ? project.index : opts.slug;
let loader = await getStaticContent(projectName, slug).catch(() => null);
if (!loader) {
rowanc1 marked this conversation as resolved.
Show resolved Hide resolved
// If you haven't loaded the first time, try the `.index`
slug = `${slug}.index`;
loader = await getStaticContent(projectName, slug).catch(() => null);
if (!loader) throw responseNoArticle();
}
const slug = opts.loadIndexPage || opts.slug == null ? project.index : opts.slug;
const loader = await getStaticContent(projectName, slug).catch(() => null);
if (!loader) throw responseNoArticle();
const footer = getFooterLinks(config, projectName, slug);
return { ...loader, footer, domain: getDomainFromRequest(request), project: projectName };
}
4 changes: 2 additions & 2 deletions themes/article/package.json
Original file line number Diff line number Diff line change
@@ -25,8 +25,8 @@
"@remix-run/node": "~1.17.0",
"@remix-run/react": "~1.17.0",
"@remix-run/vercel": "~1.17.0",
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"node-fetch": "^2.6.11",
"react": "^18.2.0",
"react-dom": "^18.2.0"
3 changes: 3 additions & 0 deletions themes/article/template.yml
Original file line number Diff line number Diff line change
@@ -52,6 +52,9 @@ options:
- type: boolean
id: numbered_references
description: Show references as numbered, rather than in APA-style. Only applies to parenthetical citations.
- type: boolean
id: folders
description: Respect nested folder structure in URL paths.
build:
install: npm install
start: npm run start
9 changes: 5 additions & 4 deletions themes/book/app/routes/$.tsx
Original file line number Diff line number Diff line change
@@ -59,11 +59,12 @@ export const meta: V2_MetaFunction<typeof loader> = ({ data, matches, location }
export const links: LinksFunction = () => [KatexCSS];

export const loader: LoaderFunction = async ({ params, request }) => {
const [first, second] = new URL(request.url).pathname.slice(1).split('/');
const projectName = second ? first : undefined;
const slug = second || first;
const [first, ...rest] = new URL(request.url).pathname.slice(1).split('/');
const config = await getConfig();
const project = getProject(config, projectName ?? slug);
const project = getProject(config, first);
const projectName = project?.slug === first ? first : undefined;
const slugParts = projectName ? rest : [first, ...rest];
const slug = slugParts.length ? slugParts.join('.') : undefined;
const flat = isFlatSite(config);
const page = await getPage(request, {
project: flat ? projectName : (projectName ?? slug),
Original file line number Diff line number Diff line change
@@ -14,17 +14,20 @@ function api404(message = 'No API route found at this URL') {
}

export const loader: LoaderFunction = async ({ request, params }) => {
const { project, slug } = params;
const [first, ...rest] = new URL(request.url).pathname
.slice(1)
.replace(/\.json$/, '')
.split('/');
// Handle /myst.xref.json as slug
if (project === undefined && slug === 'myst.xref') {
if (rest.length === 0 && first === 'myst.xref') {
const xref = await getMystXrefJson();
if (!xref) {
return json({ message: 'myst.xref.json not found', status: 404 }, { status: 404 });
}
return json(xref);
}
// Handle /myst.search.json as slug
else if (slug === 'myst.search') {
else if (rest.length === 0 && first === 'myst.search') {
const search = await getMystSearchJson();
if (!search) {
return json({ message: 'myst.search.json not found', status: 404 }, { status: 404 });
@@ -33,10 +36,10 @@ export const loader: LoaderFunction = async ({ request, params }) => {
}
const config = await getConfig();
const flat = isFlatSite(config);
const data = await getPage(request, {
project: flat ? project : (project ?? slug),
slug: flat ? slug : project ? slug : undefined,
});
const project = flat ? undefined : first;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the book theme, all the project slug stuff works correctly.

const slugParts = flat ? [first, ...rest] : rest;
const slug = slugParts.join('.');
const data = await getPage(request, { project, slug });
if (!data) return api404('No page found at this URL.');
return json(data, {
headers: {
19 changes: 14 additions & 5 deletions themes/book/app/utils/loaders.server.ts
Original file line number Diff line number Diff line change
@@ -9,7 +9,8 @@ import {
} from '@myst-theme/common';
import { redirect } from '@remix-run/node';
import { responseNoArticle, responseNoSite, getDomainFromRequest } from '@myst-theme/site';
import type { MystSearchIndex } from '@myst-theme/search';
import type { MystSearchIndex } from '@myst-theme/search';
import { slugToUrl } from 'myst-common';

const CONTENT_CDN_PORT = process.env.CONTENT_CDN_PORT ?? '3100';
const CONTENT_CDN = process.env.CONTENT_CDN ?? `http://localhost:${CONTENT_CDN_PORT}`;
@@ -63,11 +64,19 @@ export async function getPage(
const project = getProject(config, projectName);
if (!project) throw responseNoArticle();
if (opts.slug === project.index && opts.redirect) {
return redirect(projectName ? `/${projectName}` : '/');
throw redirect(projectName ? `/${projectName}` : '/');
}
if (opts.slug?.endsWith('.index') && opts.redirect) {
const newSlug = slugToUrl(opts.slug);
throw redirect(projectName ? `/${projectName}/${newSlug}` : `/${newSlug}`);
}
let slug = opts.loadIndexPage || opts.slug == null ? project.index : opts.slug;
let loader = await getStaticContent(projectName, slug).catch(() => null);
if (!loader) {
slug = `${slug}.index`;
loader = await getStaticContent(projectName, slug).catch(() => null);
if (!loader) throw responseNoArticle();
}
const slug = opts.loadIndexPage || opts.slug == null ? project.index : opts.slug;
const loader = await getStaticContent(projectName, slug).catch(() => null);
if (!loader) throw responseNoArticle();
const footer = getFooterLinks(config, projectName, slug);
return { ...loader, footer, domain: getDomainFromRequest(request), project: projectName };
}
4 changes: 2 additions & 2 deletions themes/book/package.json
Original file line number Diff line number Diff line change
@@ -25,8 +25,8 @@
"@remix-run/node": "~1.17.0",
"@remix-run/react": "~1.17.0",
"@remix-run/vercel": "~1.17.0",
"myst-common": "^1.7.2",
"myst-config": "^1.7.2",
"myst-common": "^1.7.3",
"myst-config": "^1.7.3",
"node-fetch": "^2.6.11",
"react": "^18.2.0",
"react-dom": "^18.2.0"
3 changes: 3 additions & 0 deletions themes/book/template.yml
Original file line number Diff line number Diff line change
@@ -58,6 +58,9 @@ options:
- type: boolean
id: numbered_references
description: Show references as numbered, rather than in APA-style. Only applies to parenthetical citations.
- type: boolean
id: folders
description: Respect nested folder structure in URL paths.
build:
install: npm install
start: npm run start

Unchanged files with check annotations Beta

);
const mdastStage = astStage === 'pre' ? mdastPre : mdastPost;
const { downloads, exports, parts, ...reducedFrontmatter } = frontmatter;

Check warning on line 389 in packages/myst-demo/src/index.tsx

GitHub Actions / lint

'downloads' is assigned a value but never used

Check warning on line 389 in packages/myst-demo/src/index.tsx

GitHub Actions / lint

'exports' is assigned a value but never used

Check warning on line 389 in packages/myst-demo/src/index.tsx

GitHub Actions / lint

'parts' is assigned a value but never used
return (
<figure
}
function Video({
className,

Check warning on line 41 in packages/myst-to-react/src/image.tsx

GitHub Actions / lint

'className' is defined but never used
id,
src,
urlSource,