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(create-pages): e2e test createApi and fix method+path combos #1141

Merged
merged 10 commits into from
Jan 12, 2025
27 changes: 27 additions & 0 deletions e2e/create-pages.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,5 +116,32 @@ for (const mode of ['DEV', 'PRD'] as const) {
).toBeVisible();
({ port, stopApp } = await startApp(mode));
});

test('api hi.txt', async () => {
const res = await fetch(`http://localhost:${port}/api/hi.txt`);
tylersayshi marked this conversation as resolved.
Show resolved Hide resolved
expect(res.status).toBe(200);
expect(await res.text()).toBe('hello from a text file!');
});

test('api hi', async () => {
const res = await fetch(`http://localhost:${port}/api/hi`);
expect(res.status).toBe(200);
expect(await res.text()).toBe('hello world!');
});

test('api empty', async () => {
const res = await fetch(`http://localhost:${port}/api/empty`);
expect(res.status).toBe(200);
expect(await res.text()).toBe('');
});

test('api hi with POST', async () => {
const res = await fetch(`http://localhost:${port}/api/hi`, {
method: 'POST',
body: 'from the test!',
});
expect(res.status).toBe(200);
expect(await res.text()).toBe('POST to hello world! from the test!');
});
});
}
1 change: 1 addition & 0 deletions e2e/fixtures/create-pages/private/hi.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello from a text file!
41 changes: 40 additions & 1 deletion e2e/fixtures/create-pages/src/entries.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import NestedBazPage from './components/NestedBazPage.js';
import NestedLayout from './components/NestedLayout.js';
import { DeeplyNestedLayout } from './components/DeeplyNestedLayout.js';
import ErrorPage from './components/ErrorPage.js';
import { readFile } from 'node:fs/promises';

const pages: ReturnType<typeof createPages> = createPages(
async ({ createPage, createLayout }) => [
async ({ createPage, createLayout, createApi }) => [
createLayout({
render: 'static',
path: '/',
Expand Down Expand Up @@ -99,6 +100,44 @@ const pages: ReturnType<typeof createPages> = createPages(
path: '/404',
component: () => <h2>Not Found</h2>,
}),

createApi({
path: '/api/hi.txt',
mode: 'static',
method: 'GET',
handler: async () => {
const hiTxt = await readFile('./private/hi.txt');
return new Response(hiTxt);
},
}),

createApi({
path: '/api/hi',
mode: 'dynamic',
method: 'GET',
handler: async () => {
return new Response('hello world!');
},
}),

createApi({
path: '/api/hi',
mode: 'dynamic',
method: 'POST',
handler: async (req) => {
const body = await req.text();
return new Response(`POST to hello world! ${body}`);
},
}),

createApi({
path: '/api/empty',
mode: 'static',
Copy link
Owner

Choose a reason for hiding this comment

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

It's probably a doc issue, but if we use static mode, we recommend using file extension like /api/empty.txt. Otherwise, the static build will be /api/empty/index.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can enforce that with types, but I think docs are the right way to encourage that to start.

method: 'GET',
handler: async () => {
return new Response(null);
},
}),
],
);

Expand Down
15 changes: 0 additions & 15 deletions e2e/fixtures/create-pages/src/main.tsx

This file was deleted.

73 changes: 51 additions & 22 deletions packages/waku/src/router/create-pages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,12 +150,21 @@ export type CreateLayout = <Path extends string>(

type Method = 'GET' | 'POST' | 'PUT' | 'DELETE';

export type CreateApi = <Path extends string>(params: {
path: Path;
mode: 'static' | 'dynamic';
method: Method;
handler: (req: Request) => Promise<Response>;
}) => void;
export type CreateApi = <Path extends string>(
params:
| {
path: Path;
mode: 'static';
method: 'GET';
handler: (req: Request) => Promise<Response>;
}
| {
path: Path;
mode: 'dynamic';
method: Method;
handler: (req: Request) => Promise<Response>;
},
Comment on lines +154 to +166
Copy link
Owner

Choose a reason for hiding this comment

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

Nice. We can even force file extension for path for static mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep! If we want

) => void;

type RootItem = {
render: 'static' | 'dynamic';
Expand Down Expand Up @@ -227,27 +236,26 @@ export const createPages = <
[PathSpec, FunctionComponent<any>]
>();
const apiPathMap = new Map<
string,
string, // `${method} ${path}`
Copy link
Owner

Choose a reason for hiding this comment

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

Seems room for improvement, but just a nit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have any suggestions? this composite key approach feels a tiny bit tricky to use, but it solves the problem really well

Copy link
Owner

Choose a reason for hiding this comment

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

Not right away. I'll let you know if something arises.

{
mode: 'static' | 'dynamic';
pathSpec: PathSpec;
method: Method;
handler: Parameters<CreateApi>[0]['handler'];
}
>();
const staticApiPaths = new Set<string>();
const staticComponentMap = new Map<string, FunctionComponent<any>>();
let rootItem: RootItem | undefined = undefined;
const noSsrSet = new WeakSet<PathSpec>();

/** helper to find dynamic path when slugs are used */
const getRoutePath: (path: string) => string | undefined = (path) => {
const getPageRoutePath: (path: string) => string | undefined = (path) => {
if (staticComponentMap.has(joinPath(path, 'page').slice(1))) {
return path;
}
const allPaths = [
...dynamicPagePathMap.keys(),
...wildcardPagePathMap.keys(),
...apiPathMap.keys(),
];
for (const p of allPaths) {
if (getPathMapping(parsePathWithSlug(p), path)) {
Expand All @@ -256,12 +264,29 @@ export const createPages = <
}
};

const pathExists = (path: string) => {
const getApiRoutePath: (
path: string,
method: string,
) => string | undefined = (path, method) => {
for (const pathKey of apiPathMap.keys()) {
const [m, p] = pathKey.split(' ');
if (m === method && getPathMapping(parsePathWithSlug(p!), path)) {
return p;
}
}
};

const pagePathExists = (path: string) => {
for (const pathKey of apiPathMap.keys()) {
const [_m, p] = pathKey.split(' ');
if (p === path) {
return true;
}
}
return (
staticPathMap.has(path) ||
dynamicPagePathMap.has(path) ||
wildcardPagePathMap.has(path) ||
apiPathMap.has(path)
wildcardPagePathMap.has(path)
);
};

Expand Down Expand Up @@ -290,7 +315,7 @@ export const createPages = <
if (configured) {
throw new Error('createPage no longer available');
}
if (pathExists(page.path)) {
if (pagePathExists(page.path)) {
throw new Error(`Duplicated path: ${page.path}`);
}

Expand Down Expand Up @@ -388,12 +413,16 @@ export const createPages = <
if (configured) {
throw new Error('createApi no longer available');
}
if (apiPathMap.has(path)) {
throw new Error(`Duplicated api path: ${path}`);
if (apiPathMap.has(`${method} ${path}`)) {
throw new Error(`Duplicated api path+method: ${path} ${method}`);
} else if (mode === 'static' && staticApiPaths.has(path)) {
throw new Error('Static API Routes cannot share paths: ' + path);
tylersayshi marked this conversation as resolved.
Show resolved Hide resolved
}
if (mode === 'static') {
staticApiPaths.add(path);
}

const pathSpec = parsePathWithSlug(path);
apiPathMap.set(path, { mode, pathSpec, method, handler });
apiPathMap.set(`${method} ${path}`, { mode, pathSpec, handler });
};

const createRoot: CreateRoot = (root) => {
Expand Down Expand Up @@ -530,7 +559,7 @@ export const createPages = <
await configure();

// path without slugs
const routePath = getRoutePath(path);
const routePath = getPageRoutePath(path);
if (!routePath) {
throw new Error('Route not found: ' + path);
}
Expand Down Expand Up @@ -607,11 +636,11 @@ export const createPages = <
},
handleApi: async (path, options) => {
await configure();
const routePath = getRoutePath(path);
const routePath = getApiRoutePath(path, options.method);
if (!routePath) {
throw new Error('Route not found: ' + path);
throw new Error('API Route not found: ' + path);
}
const { handler } = apiPathMap.get(routePath)!;
const { handler } = apiPathMap.get(`${options.method} ${routePath}`)!;

const req = new Request(
new URL(
Expand Down
Loading