-
-
Notifications
You must be signed in to change notification settings - Fork 132
Test 404 delegation in @fedify/solidstart #1011
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,7 +49,8 @@ | |
| "build:self": "tsdown", | ||
| "build": "pnpm --filter @fedify/solidstart... run build:self", | ||
| "prepack": "pnpm build", | ||
| "prepublish": "pnpm build" | ||
| "prepublish": "pnpm build", | ||
| "test": "node --experimental-transform-types --test" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add the |
||
| }, | ||
| "peerDependencies": { | ||
| "@fedify/fedify": "workspace:^", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import type { Federation } from "@fedify/fedify/federation"; | ||
| import type { FetchEvent } from "@solidjs/start/server"; | ||
|
|
||
| /** | ||
| * A factory function that creates the context data for the | ||
| * {@link Federation} object. | ||
| * | ||
| * @template TContextData The type of the context data. | ||
| * @param event The SolidStart {@link FetchEvent} for the current request. | ||
| * @returns The context data, or a promise resolving to the context data. | ||
| * @since 2.2.0 | ||
| */ | ||
| export type ContextDataFactory<TContextData> = ( | ||
| event: FetchEvent, | ||
| ) => TContextData | Promise<TContextData>; | ||
|
|
||
| // Internal storage for 406 Not Acceptable responses across the | ||
| // onRequest -> onBeforeResponse lifecycle, keyed by Request object. | ||
| const notAcceptableResponses: WeakMap<Request, Response> = new WeakMap(); | ||
|
|
||
| /** | ||
| * Creates the `onRequest` handler wired into SolidStart by | ||
| * `fedifyMiddleware()`. | ||
| * | ||
| * @template TContextData A type of the context data for the | ||
| * {@link Federation} object. | ||
| * @param federation A {@link Federation} object to integrate with SolidStart. | ||
| * @param createContextData A function to create context data for the | ||
| * {@link Federation} object. | ||
| * @returns The `onRequest` handler. | ||
| */ | ||
| export function createOnRequestHandler<TContextData>( | ||
| federation: Federation<TContextData>, | ||
| createContextData: ContextDataFactory<TContextData>, | ||
| ): (event: FetchEvent) => Promise<Response | undefined> { | ||
| return async (event: FetchEvent) => { | ||
| const response = await federation.fetch(event.request, { | ||
| contextData: await createContextData(event), | ||
| onNotFound: () => new Response("Not Found", { status: 404 }), | ||
| onNotAcceptable: () => | ||
| new Response("Not Acceptable", { | ||
| status: 406, | ||
| headers: { "Content-Type": "text/plain", Vary: "Accept" }, | ||
| }), | ||
| }); | ||
|
|
||
| // If Fedify does not handle this route, let SolidStart handle it: | ||
| if (response.status === 404) return; | ||
|
|
||
| // If content negotiation failed (client does not want JSON-LD), | ||
| // store the 406 response and let SolidStart try to serve HTML. | ||
| // If SolidStart also cannot handle it, onBeforeResponse will | ||
| // return the 406: | ||
| if (response.status === 406) { | ||
| notAcceptableResponses.set(event.request, response); | ||
| return; | ||
| } | ||
|
Comment on lines
+47
to
+57
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not the changes by this PR, but checking |
||
|
|
||
| // Fedify handled the request successfully: | ||
| return response; | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Creates the `onBeforeResponse` handler wired into SolidStart by | ||
| * `fedifyMiddleware()`. | ||
| * | ||
| * @returns The `onBeforeResponse` handler. | ||
| */ | ||
| export function createOnBeforeResponseHandler(): ( | ||
| event: FetchEvent, | ||
| ) => Response | undefined { | ||
| // Similar to onRequest, but slightly more tricky one. | ||
| // When the federation object finds a request not acceptable type-wise | ||
| // (i.e., a user-agent does not want JSON-LD), onRequest stores the 406 | ||
| // response and lets SolidStart try to render HTML. If SolidStart also | ||
| // has no page for this route (404), we return the stored 406 instead. | ||
| // This enables Fedify and SolidStart to share the same routes and do | ||
| // content negotiation depending on the Accept header: | ||
| return (event: FetchEvent) => { | ||
| const stored = notAcceptableResponses.get(event.request); | ||
| if (stored != null) { | ||
| notAcceptableResponses.delete(event.request); | ||
| const status = event.response.status ?? 200; | ||
| if (status === 404) return stored; | ||
| } | ||
| }; | ||
| } | ||
|
Comment on lines
+70
to
+88
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { createFederation, MemoryKvStore } from "@fedify/fedify"; | ||
| import type { FetchEvent } from "@solidjs/start/server"; | ||
| import { strict as assert } from "node:assert"; | ||
| import { describe, test } from "node:test"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please consider using |
||
| import { createOnRequestHandler } from "./handlers.ts"; | ||
|
|
||
| // The handlers are the SUT instead of fedifyMiddleware(): importing it | ||
| // would load @solidjs/start's *.jsx* runtime modules, which need a bundler. | ||
| describe("[solidstart] fedifyMiddleware()", () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This label said |
||
| test("returns no response when federation reports not-found via onNotFound", async () => { | ||
| // No dispatcher is registered, so federation.fetch() takes the | ||
| // onNotFound path for every request: | ||
| const federationWithoutDispatcher = createFederation<void>({ | ||
| kv: new MemoryKvStore(), | ||
| }); | ||
| let contextDataFactoryCalls = 0; | ||
| const onRequest = createOnRequestHandler( | ||
| federationWithoutDispatcher, | ||
| () => { | ||
| contextDataFactoryCalls++; | ||
| }, | ||
| ); | ||
|
|
||
| const event = { | ||
| request: new Request("http://localhost/hello-world"), | ||
| locals: {}, | ||
| } as unknown as FetchEvent; | ||
| const response = await onRequest(event); | ||
|
|
||
| assert.strictEqual( | ||
| contextDataFactoryCalls, | ||
| 1, | ||
| "the context data factory must be consulted for the request", | ||
| ); | ||
|
Comment on lines
+30
to
+34
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this assertion is too much? |
||
| assert.strictEqual( | ||
| response, | ||
| undefined, | ||
| "onRequest must not return a response when Fedify reports not-found " + | ||
| "so that SolidStart can handle the request", | ||
| ); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: fedify-dev/fedify
Length of output: 7795
🏁 Script executed:
Repository: fedify-dev/fedify
Length of output: 10629
🏁 Script executed:
Repository: fedify-dev/fedify
Length of output: 39040
🏁 Script executed:
Repository: fedify-dev/fedify
Length of output: 33586
🏁 Script executed:
Repository: fedify-dev/fedify
Length of output: 2339
Remove unrestricted Deno permissions.
Run the package test with
deno testinstead ofdeno test --allow-all. The tested no-dispatcher path uses in-memory storage and does not perform network I/O. Add specific permissions only when a test requires them.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 2401
🏁 Script executed:
Length of output: 378
🏁 Script executed:
Length of output: 596
@2chanhaeng, the requested change is not present.packages/solidstart/deno.jsonstill defines:The configuration has no local diff. Validation also could not run because
miseis not installed in the sandbox. Please change the task to"deno test"and rerun validation in an environment withmiseavailable.You are interacting with an AI system.