feat: make root optional when serve is false #460 #536
Conversation
|
@Dipali127 The index.js change is missing. |
|
@Fdawgs I reverted the stylistic changes as requested. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for making the root option optional when serve: false is specified in fastifyStatic, allowing users to skip providing a dummy directory when they only want to use sendFile functionality.
- Conditionally skip root validation when
serve: false - Add comprehensive tests to verify both serving and non-serving behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| index.js | Added conditional logic to skip root validation when serve is false |
| test/serve-option.test.js | New test file to verify serve option behavior with and without static file serving |
| test/root/example.html | Test fixture HTML file for serving tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Hi @Fdawgs, I’ve resolved all the Copilot comments and fixed the lint issues. The PR is now ready for final review and merge. 😊 |
|
The type should also be updated if the Also, could you please confirm that the issues mentioned in the comment below are resolved by your changes?
|
| opts.root = normalizeRoot(opts.root) | ||
| checkRootPathForErrors(fastify, opts.root) | ||
| if (opts.serve !== false) { | ||
| opts.root = normalizeRoot(opts.root) |
There was a problem hiding this comment.
@Dipali127
Why should normalizeRoot also be skipped?
There was a problem hiding this comment.
In this scenario we are not using root if I'm not wrong
|
Hi @is2ei 👋, thank you for the feedback! I’ve updated the PR with the following changes and clarifications: Type update: Regarding skipping About issue #467: Let me know your thoughts — I can update the |
|
Thanks @Dipali127, could you update the types tests and then i'm happy. Thanks for taking a look @is2ei. |
|
Could you please disclose any use of AI usage for this PR? |
I though so over here in #537 (comment) as the em-dash is suspect but wanted to give benefit of the doubt. |
|
|
||
| export interface FastifyStaticOptions extends SendOptions { | ||
| root: string | string[] | URL | URL[]; | ||
| root?: string | string[] | URL | URL[]; |
There was a problem hiding this comment.
Boring to write, but we need something like:
export interface FastifyStaticOptions extends SendOptions {
...stuff
} & ({
serve: false,
} | {
serve: true,
root: string | string[] | URL | URL[];
})
| opts.root = normalizeRoot(opts.root) | ||
| checkRootPathForErrors(fastify, opts.root) | ||
| if (opts.serve !== false) { | ||
| opts.root = normalizeRoot(opts.root) |
There was a problem hiding this comment.
In this scenario we are not using root if I'm not wrong
|
|
||
| t.after(() => fastify.close()) | ||
| await fastify.listen({ port: 0 }) | ||
| fastify.server.unref() |
There was a problem hiding this comment.
| fastify.server.unref() | |
| t.after(() => fastify.close()) |
|
|
||
| t.after(() => fastify.close()) | ||
| await fastify.listen({ port: 0 }) | ||
| fastify.server.unref() |
There was a problem hiding this comment.
| fastify.server.unref() | |
| t.after(() => fastify.close()) |
|
I close this in favor of #540 |
6 participants
Summary
This PR makes the
rootoption optional whenserve: falseinfastifyStaticasynchronous function.Previously, a dummy directory was required just to satisfy validation.
Changes
index.jsto skip root validation ifserve: false.serve-option.test.jsto verify behavior with and without serving.Tests
serve: false→ no automatic routes are created, sendFile still works.serve: true→ normal static file serving works as expected.Fixes #460