-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Add Support for Serving Embedded Static Files #3448
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: v2
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughA new method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Router
participant FSHandler
User->>App: Call StaticFilesystem(prefix, filesystem, config)
App->>Router: registerStaticFS(prefix, filesystem, config)
Router->>FSHandler: Setup handler with config
User->>App: HTTP GET /static/file
App->>FSHandler: Serve file from fs.FS
FSHandler-->>User: Return file content or next middleware
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 3
🧹 Nitpick comments (5)
app.go (1)
17-18
: Unnecessary extra alias creates import-group churn
app.go
imports the sameio/fs
package thatrouter.go
already brings in (aliased asiofs
). Having two different aliases (fs
vsiofs
) across the project makes grepping harder and quickly leads to style drift.-import ( - … - "io/fs" +import ( + … + "io/fs" // TODO: decide on **one** project-wide alias, e.g. “iofs” or plain “fs”router.go (4)
10-11
: Choose a single alias forio/fs
The package is imported here as
iofs
but as plainfs
inapp.go
. Pick one form to improve readability and keep IDE-based import grouping stable.
448-565
: Large copy–paste block duplicatesregisterStatic
; extract common helper
registerStaticFS
is ~120 lines that are almost byte-for-byte identical toregisterStatic
, differing only in:
- the source of the files (
FS
vsRoot
)- the default
Root
value ("."
)- the parameter types
Duplicating this logic multiplies maintenance cost (bug fixes, new flags, security tweaks). Consider extracting the shared parts into a single unexported helper that accepts an
*fasthttp.FS
already pre-configured, e.g.:func (app *App) mountStaticHandler(prefix string, fsConf *fasthttp.FS, cfg ...Static)Both
registerStatic
andregisterStaticFS
would then build thefasthttp.FS
struct (differing in only two fields) and delegate the rest.Benefits:
- One canonical path-rewrite algorithm
- One place to toggle new
Static
options- Smaller diff-footprint for future features
468-478
:Root: "."
is ignored whenFS
is set – remove to avoid confusionAccording to
fasthttp.FS
docs, when theFS
field is non-nil theRoot
field is ignored. LeavingRoot: "."
in place is harmless but can confuse future readers into thinking the path is relevant.- FS: filesystem, - Root: ".", + FS: filesystem,
500-517
: Nil-pointer guard for caller mistakes
registerStaticFS
dereferencesconfig[0]
multiple times without a length check after the first guard.
If the caller passes an empty slice and later iterations of the code are changed, this could regress.Adding a short nil-safe accessor avoids the hazard:
var userCfg Static if len(config) > 0 { userCfg = config[0] }and replace every
config[0].Foo
withuserCfg.Foo
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app.go
(2 hunks)router.go
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app.go (1)
router.go (2)
App
(207-229)Router
(23-47)
// StaticFilesystem Supports Serving Static Resources from an Embed.FS File System | ||
// | ||
// The filesystem parameter can directly accept an embed.FS instance. | ||
func (app *App) StaticFilesystem(prefix string, filesystem fs.FS, config ...Static) Router { | ||
app.registerStaticFS(prefix, filesystem, config...) | ||
return app | ||
} |
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.
💡 Verification agent
🧩 Analysis chain
Expose the new helper through the public Router interface
StaticFilesystem
is now public on *App
, but the Router
interface (defined in router.go
, lines 22-47) hasn’t been extended.
As a result:
var r fiber.Router = app // common pattern
r.StaticFilesystem("/assets", myFS) // ← does **not** compile
The omission also means *Group
cannot forward the call, limiting feature parity with Static
.
type Router interface {
…
Static(prefix, root string, config ...Static) Router
+ StaticFilesystem(prefix string, filesystem fs.FS, config ...Static) Router
…
}
After extending the interface, forwarders for *Group
can be added with two lines:
func (g *Group) StaticFilesystem(p string, fs fs.FS, cfg ...Static) Router {
return g.app.StaticFilesystem(getGroupPath(g.Prefix, p), fs, cfg...)
}
🏁 Script executed:
#!/bin/bash
set -e
# Locate the Router interface declaration and its methods
rg "type Router interface" -A 20
# Confirm if StaticFilesystem is already declared in router.go
rg "StaticFilesystem" -n router.go || echo "No StaticFilesystem in router.go"
# Inspect Group type and existing Static methods
rg "type Group" -A 10
rg "func .*Group\).*Static" -n
Length of output: 1560
Expose StaticFilesystem on the Router interface and Group
The new StaticFilesystem
helper is public on *App
but isn’t declared on the Router
interface (router.go) or forwarded by *Group
(group.go), preventing common patterns like:
var r fiber.Router = app
r.StaticFilesystem("/assets", myFS) // does not compile
Please add:
• router.go (within type Router interface
):
type Router interface {
…
Static(prefix, root string, config ...Static) Router
+ StaticFilesystem(prefix string, filesystem fs.FS, config ...Static) Router
…
}
• group.go (alongside func (grp *Group) Static
):
func (grp *Group) StaticFilesystem(p string, fs fs.FS, cfg ...Static) Router {
return grp.app.StaticFilesystem(getGroupPath(grp.Prefix, p), fs, cfg...)
}
// Don't execute middleware if Next returns true | ||
if len(config) != 0 && config[0].Next != nil && config[0].Next(c) { | ||
return c.Next() | ||
} | ||
// Serve file | ||
fileHandler(c.fasthttp) | ||
// Sets the response Content-Disposition header to attachment if the Download option is true | ||
if len(config) > 0 && config[0].Download { | ||
c.Attachment() | ||
} | ||
// Return request if found and not forbidden | ||
status := c.fasthttp.Response.StatusCode() | ||
if status != StatusNotFound && status != StatusForbidden { | ||
if len(cacheControlValue) > 0 { | ||
c.fasthttp.Response.Header.Set(HeaderCacheControl, cacheControlValue) | ||
} | ||
if modifyResponse != nil { | ||
return modifyResponse(c) | ||
} | ||
return nil | ||
} | ||
// Reset response to default | ||
c.fasthttp.SetContentType("") // Issue #420 | ||
c.fasthttp.Response.SetStatusCode(StatusOK) | ||
c.fasthttp.Response.SetBodyString("") | ||
// Next middleware | ||
return c.Next() | ||
} |
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.
Missing nil-filesystem validation
If a caller accidentally passes a nil fs.FS
, the fast-http handler panics the first time it tries to open a file.
if filesystem == nil {
panic("StaticFilesystem: filesystem cannot be nil")
}
Fail fast with a clear message rather than a runtime panic deep inside fasthttp
.
use: true, | ||
root: isRoot, | ||
path: prefix, | ||
// Public data | ||
Method: MethodGet, | ||
Path: prefix, | ||
Handlers: []Handler{handler}, | ||
} |
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.
🛠️ Refactor suggestion
Route metadata sets use: true
but only registers GET/HEAD
use: true
traditionally means match all HTTP methods.
Because you later add the route only to GET & HEAD stacks, the flag is misleading for downstream tools that rely on it (e.g. route printers, metrics).
Set use: false
or register the handler for all methods, mirroring the behaviour of app.Static
.
Thx for the effort Is everything go 1.17 ready? V2 is supporting this old version |
@miseelu can you provide tests for this feature |
@miseelu just so you know, i'm currently trying to reduce the custom methods for adding routes in this issue the idea behind this is not to create the route struct in different places |
@miseelu Please provide Unit-Tests and update the Documentation |
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.
Pull Request Overview
This PR adds a new StaticFilesystem
method to serve embedded static files from an embed.FS
, aligning with Fiber’s existing static file API.
- Introduces
registerStaticFS
inrouter.go
to handle embed.FS file serving - Adds
StaticFilesystem
wrapper inapp.go
- Keeps backward compatibility with the original
Static
method
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
router.go | Implements registerStaticFS with path rewriting and handler registration for embedded FS |
app.go | Exposes StaticFilesystem on App to call registerStaticFS |
Comments suppressed due to low confidence (3)
router.go:448
- [nitpick] Consider adding unit tests for
registerStaticFS
to cover path rewriting logic and embedded FS handling to ensure correct behavior.
func (app *App) registerStaticFS(prefix string, filesystem iofs.FS, config ...Static) {
router.go:476
- The time package is not imported, causing a build error. Add
import "time"
at the top of the file.
CacheDuration: 10 * time.Second,
router.go:560
- The sync/atomic package is not imported, causing a build error. Add
import "sync/atomic"
at the top of the file.
atomic.AddUint32(&app.handlersCount, 1)
// StaticFilesystem Supports Serving Static Resources from an Embed.FS File System | ||
// | ||
// The filesystem parameter can directly accept an embed.FS instance. |
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.
[nitpick] The doc comment should follow Go conventions and include usage examples. For example:
// StaticFilesystem serves embedded static files from an fs.FS.
// Example:
// app.StaticFilesystem("/assets", embeddedFiles)
// StaticFilesystem Supports Serving Static Resources from an Embed.FS File System | |
// | |
// The filesystem parameter can directly accept an embed.FS instance. | |
// StaticFilesystem serves static resources from an fs.FS, such as an embed.FS instance. | |
// | |
// Example: | |
// // Embed static files using embed.FS | |
// //go:embed static/* | |
// var embeddedFiles embed.FS | |
// | |
// app.StaticFilesystem("/static", embeddedFiles) |
Copilot uses AI. Check for mistakes.
} | ||
|
||
// Create route metadata without pointer | ||
route := Route{ |
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.
@miseelu can you check our last hints |
Description
This pull request introduces a new feature to Fiber v2 that enables serving static files directly from an embedded filesystem (embed.FS). The enhancement allows developers to bundle static resources with their Go binary using Go's embed functionality, eliminating the need for external file dependencies in production.
The main change adds a new StaticFilesystem method that accepts an embed.FS instance, making it seamless to serve embedded static files while maintaining consistency with Fiber's existing static file serving API.
Added StaticFilesystem method to support serving static resources from embed.FS
Implemented registerStaticFS helper function in router.go
Maintained backward compatibility with existing static file serving functionality
Fixes # (issue)
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/
directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md