search

knadh / niltalk

Instant, disposable, single-binary web based live chat server. Go + VueJS.
GNU Affero General Public License v3.0
948 stars 118 forks source link

Switch stuffbin for go.rice to embed static assets #24

Closed clementauger closed 3 years ago

knadh commented 3 years ago

Why is this being switched?

clementauger commented 3 years ago

that helps to improve development scenarios. Using stuffbin i had to restart the app to apply changes to the template or assets. And this behaviour was propagated to an user that would like to develop new templates/assets using a built binary.

Switching to rice it is possible to improve the developer experience via their mechanism that checks for FS before embedded resources.

The other change is that using go run . the templates automatically auto rebuild, unless the flag --jit=false is provided. Using a built binary the jit default is set to false in order to prefer optimized templates.

There is obviously an additional overhead regarding the FS lookups, though, i don't see that as a problem in regard to the improved experience.

clementauger commented 3 years ago

Though, this is breaking because the flag static-dir has disappeared!

It also moves samples to their own folder under static directory, though this is transparent and it is being made in preparation for systemd helpers.

knadh commented 3 years ago

I'd written stuffbin after evaluating go.rice and a few other libs to get better control on the filesystem abstraction, like seamlessly merging different FS (memory, disk etc.), being able to extend and define custom file systems, and also controlling aliasing of files which is very useful in embedding static resources, eg, making /home/my/code/stuff/frontend/assets/static/ -> /static in the virtual FS.

stuffbin doesn't have a live-load implementation right now, but it can be easily added. I've however refrained from doing that because Go is about to get not only file embedding support, but an actual FS abstraction in its core.

Thus, I don't think we should swap out stuffbin for another lib and should rather just wait for the release of Go/embed/FS, especially given that the sole benefit right now is of minor added convenience when occasionally tweaking the frontend code.

knadh commented 3 years ago

To add, if you could undo this switch, we could start merging the other feature PRs one by one. Thanks!

clementauger commented 3 years ago

Hi! Sorry to hear that. This is a no-go for me. imho, those are not minor improvements and i am seeking for solutions to get things done, not to re invent the wheel, thus updating stuffbin was not option to me.

I was also waiting after this merge to send out a few more changes that are relying on this.. so sorry that we can not find a suitable working ground.

anyway, i will still help for other PR, if needed, though they are rather simple.

knadh commented 3 years ago

It would be shame if the other PRs (which are excellent) were stuck because of this. Let me see if I can quickly add live-load to stuffbin so that this could work with maybe a one-line change without swapping out the libs. Would that work for the other PRs?

clementauger commented 3 years ago

for current PRs i am pretty sure we dont need to find a solution around this, i specifically branched and backported them from your last commit.

no urgency on that side.

I re collected changes on my side, i need this to add helpers for systemd service setup, and maybe macos (windows is yet another story). I was also starting to plan on theming and it was working smoothly in my head with go.rice.

More generally my working setup is a bit messy and having this merged would have helped to settle things down.

knadh commented 3 years ago

If you've put in so much work, it doesn't make sense to undo it and we could just go with go.rice now and replace it with Go/embed in the future. Let me look at this again. Theming is already supported with --static-dir though.

clementauger commented 3 years ago

that one ? https://github.com/pyros2097/go-embed

take care static-dir was removed in this PR. This is breaking change.

I was thinking about theming end user could change via a web UI and cookie supports.

knadh commented 3 years ago

that one ? https://github.com/pyros2097/go-embed

No. Go will soon get file embedding support in its core. See https://github.com/golang/go/issues/35950. We ideally wouldn't need exhaustive libraries like go.rice and stuffbin once it's out.

take care static-dir was removed in this PR

Ah, why is that? It's a useful feature for users who want to make custom changes to the frontend, heavy or light.

clementauger commented 3 years ago

Ah! I did not know about that possible feature of the core. Well It won t be there before 6 months, at least, and i am unclear if that will handle my use case of JIT reload out of the box or if something will have to be done on top of it, delaying that much any move on that PR question.

For static-dir, that did not seem applicable / useful anymore when i applied the changes, though now i know better go.rice i think it can be handled. I only need to test and implement.

clementauger commented 3 years ago

FTR, it is not possible to keep that flag because when you try to setup a rice.box from a dynamic path, the build fails with

main.go:167: Error: found call to rice.FindBox, but argument must be a string literal.

knadh commented 3 years ago

Ah, this is a problem. --static-dir is an important feature to let users customize the frontends easily without having to recompile the Go app. I still don't think the switch to go.rice just for live-reload during dev is warranted ;(

clementauger commented 3 years ago

just in case we have misunderstandings, the point of this change is to be able to update the frontend without re compiling the app. The only difference is that the user can not change the static-dir path anymore, it is always [working dir]/static-dir

using stuffbin i always have to rerun the app to make the app take in account the changes. anyways.