Add reload.FS for hot reloading during development

[fasmat]

This adds an implementation of fs.FS that can do both: hot reload and embedding of files. embed.go is automatically hidden from the embedded file system.

There is a related PR here: gobuffalo/cli#60

This PR updates the new command to generate embed.go files that use the reload.FS.

[sio4]

Hi @fasmat, nice work!
I added some comments and suggestions. Please consider them.

[sio4]

I tested creating a new app with this PR along with gobuffalo/cli#60 and the result was excellent! the live loading worked perfectly as before!

I think we can merge it once we agreed with the name of the module, and some minor bugs could be managed after this hotfix was released.

[fasmat]

I tested creating a new app with this PR along with gobuffalo/cli#60 and the result was excellent! the live loading worked perfectly as before!

I think we can merge it once we agreed with the name of the module, and some minor bugs could be managed after this hotfix was released.

How about we move this functionality to the root of the gobuffalo/buffalo project? Then the name would be buffalo.FS which seems fitting to me?

[sio4]

How about we move this functionality to the root of the gobuffalo/buffalo project? Then the name would be buffalo.FS which seems fitting to me?

Wow! I love the name!

[sio4]

Nice work! I hope we can have it as soon as possible!

[paganotoni]

Hey Folks! This is exciting, as for the hot reloading mechanics, just to confirm:

• while in development mode GO_ENV=development this will use a Dir FS.
• while in production mode GO_ENV=production this will use the embedded FS

Am I right with those assumptions?

[paganotoni]

@fasmat I love it! It's really exciting to see this progress! Adding here my 5 cents:

I would suggest to add some tests to buffalo.FS as it will be an important (and very much used) element in gobuffalo/* and users codebases.

Also, and to add some context, there was a security issue in Packr we should keep in mind here. At some point Packr was allowing to read relative folders, opening the entire docker container to the world. This was a security issue as things like environment variables usually are stored in OSs files.

As for how this should work we need to make our buffalo.FS consider the environment variable (GO_ENV) and only use disk FS when development or test. Otherwise always use the embedded folder.

[fasmat]

Hey Folks! This is exciting, as for the hot reloading mechanics, just to confirm:

• while in development mode GO_ENV=development this will use a Dir FS.
• while in production mode GO_ENV=production this will use the embedded FS

Am I right with those assumptions?

No this is not true. It behaves similarly to how packr functioned: if the requested files can be found on the disk, those will be used. Otherwise it will use the embedded files.

EDIT: there are now also tests that check for exactly this behavior.

[fasmat]

@fasmat I love it! It's really exciting to see this progress! Adding here my 5 cents:

I would suggest to add some tests to buffalo.FS as it will be an important (and very much used) element in gobuffalo/* and users codebases.

@paganotoni Thanks for the feedback 😄 I'll see what I can do. The functionality is very limited. The whole thing is just 2 methods that basically just override the functions of embed.FS and os.DirFS to hide embed.go.

Also, and to add some context, there was a security issue in Packr we should keep in mind here. At some point Packr was allowing to read relative folders, opening the entire docker container to the world. This was a security issue as things like environment variables usually are stored in OSs files.

As for how this should work we need to make our buffalo.FS consider the environment variable (GO_ENV) and only use disk FS when development or test. Otherwise always use the embedded folder.

I don't think this is necessary @sio4 mentioned the use case of not having embedded files in production, which used to work with packr and maybe some other users also rely on. Regarding the security issue with relative folders: both embed.FS and os.DirFS don't allow this. There is no way that buffalo.FS would introduce this bug by just wrapping those two.

EDIT: embed.FS only contains the files that were embedded using the //go:embed directive and os.DirFS checks the given path with fs.ValidPath (see here). The documentation for it says Path names must not contain an element that is “.” or “..” or the empty string (see here)

EDIT2: just to be 100% certain that this is really the case I added a test that checks for exactly this. As I expected no .. is allowed to be used in the path to a file.

[paganotoni]

ok. Thanks @fasmat, It's great to know that stdlib is covering that issue. I would suggest that as part of testing the fallback behavior we're implementing in buffalo.FS we add some of those relative paths test, maybe just one to have as a guarantee of what we expect from this FS implementation.

[paganotoni]

Awesome. Thanks @fasmat !