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(fiber): Packaged assets not being served #265

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

mubashiroliyantakath
Copy link
Contributor

@mubashiroliyantakath mubashiroliyantakath commented Jun 19, 2024

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

Problem/Feature

Observed that after building the final output for a project with HTMX and Fiber Server, the files in the embedded assets were not being properly served to the client (404 not found error for the htmx javascript file and the styles file).

Description of Changes:

  • Used the fiber filesystem middleware to serve the required files

Checklist

  • I have self-reviewed the changes being requested

@mubashiroliyantakath mubashiroliyantakath changed the title fix(fiber): Packaged assets not being served when fix(fiber): Packaged assets not being served Jun 19, 2024
@@ -1,4 +1,8 @@
s.App.Static("/assets", "./cmd/web/assets")
s.App.Use("/assets", filesystem.New(filesystem.Config{
Copy link
Contributor

Choose a reason for hiding this comment

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

No, you don't have to use FS. The issue is with your OS/machine package. The asset (static files) is not being served because FS already has the magic embedded here:

package web

import "embed"

//go:embed "assets"
var Files embed.FS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking into it.

Although we are declaring the embed.FS variable Files, this is not actually being referred to in the original s.App.Static() method. So, I believe the method was looking in the actual file system instead. This makes sense because it was working fine on my local development environment. But once I built it and ran the binary in an isolated environment, I ran into the issue of the assets not being there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking into it.

Although we are declaring the embed.FS variable Files, this is not actually being referred to in the original s.App.Static() method. So, I believe the method was looking in the actual file system instead. This makes sense because it was working fine on my local development environment. But once I built it and ran the binary in an isolated environment, I ran into the issue of the assets not being there.

It's because the path/directory is different.

Here's an example of how to use static files with magic embedding:

The static.go file is located in the frontend/public directory:

package frontend

import "embed"

// Files is an embedded file system containing the static files from the "images" directory.
//
// Note: This is a "magic embedded" line and should not be removed, as it is initialized before other code (even another FS System).
//
//go:embed "assets/images"
var Files embed.FS

Then, in Fiber, you can call it like this:

	app.Static("/styles/", "./frontend/public/assets", fiber.Static{
		Compress: true,
		// Note: It's important to disable this when using middleware cache to avoid confusion,
		// as caching is already handled by the middleware cache.
		CacheDuration: -1 * time.Microsecond,
	})

It works well for me on Linux, Windows, and Unix systems without using a file system such as the Fiber file manager (e.g., filesystem.New()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I found in the official Fiber docs.

// Embed a single file
//go:embed index.html
var f embed.FS

// Embed a directory
//go:embed static/*
var embedDirStatic embed.FS

func main() {
	app := fiber.New()

	app.Use("/", filesystem.New(filesystem.Config{
		Root: http.FS(f),
	}))

	// Access file "image.png" under `static/` directory via URL: `http://<server>/static/image.png`.
	// Without `PathPrefix`, you have to access it via URL:
	// `http://<server>/static/static/image.png`.
	app.Use("/static", filesystem.New(filesystem.Config{
		Root: http.FS(embedDirStatic),
		PathPrefix: "static",
		Browse: true,
	}))

	log.Fatal(app.Listen(":3000"))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I found in the official Fiber docs.

// Embed a single file
//go:embed index.html
var f embed.FS

// Embed a directory
//go:embed static/*
var embedDirStatic embed.FS

func main() {
	app := fiber.New()

	app.Use("/", filesystem.New(filesystem.Config{
		Root: http.FS(f),
	}))

	// Access file "image.png" under `static/` directory via URL: `http://<server>/static/image.png`.
	// Without `PathPrefix`, you have to access it via URL:
	// `http://<server>/static/static/image.png`.
	app.Use("/static", filesystem.New(filesystem.Config{
		Root: http.FS(embedDirStatic),
		PathPrefix: "static",
		Browse: true,
	}))

	log.Fatal(app.Listen(":3000"))
}

@mubashiroliyantakath The alternative without using the filesystem middleware works. The only thing you need to do is put the magic embed before the other Go code. Here's an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

and I've been using it in my template.

@Ujstor
Copy link
Collaborator

Ujstor commented Jun 21, 2024

@mubashiroliyantakath This is good, chach. It is hard sometimes to test everything. Implementing endpoint tests in CI would be a good thing. Tested and works as expected.

@H0llyW00dzZ any suggestions before I approve and merge? I know you use Fiber extensively

@H0llyW00dzZ
Copy link
Contributor

H0llyW00dzZ commented Jun 23, 2024

@mubashiroliyantakath This is good, chach. It is hard sometimes to test everything. Implementing endpoint tests in CI would be a good thing. Tested and works as expected.

@H0llyW00dzZ any suggestions before I approve and merge? I know you use Fiber extensively

@Ujstor The current Root setting in the code is a bit risky due to the Browse: true path behavior.

	app.Use("/static", filesystem.New(filesystem.Config{
		Root: http.FS(embedDirStatic),
		PathPrefix: "static",
		Browse: true,
	}))

If a user requests http://<server>/static1/static2/image.png, setting the Root to static2 would be safe for serving images like image.png or other files directly within the static2 directory. However, this configuration could potentially lead to issues if users attempt to access files or directories outside of the static2 directory within the static1 path because Fiber's CaseSensitive and StrictRouting options are disabled by default.

Copy link
Collaborator

@Ujstor Ujstor left a comment

Choose a reason for hiding this comment

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

I am leaving it to the user to do more granular configuration, assuming that they know what they are doing.

LGTM

@Ujstor Ujstor merged commit a2ee125 into Melkeydev:main Jul 8, 2024
127 checks passed
@mubashiroliyantakath mubashiroliyantakath deleted the package-assets branch July 8, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants