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

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

Closed
wants to merge 2 commits into from

Conversation

clementauger
Copy link
Contributor

No description provided.

@knadh
Copy link
Owner

knadh commented Aug 25, 2020

Why is this being switched?

@clementauger
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

knadh commented Aug 30, 2020

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 knadh closed this Aug 30, 2020
@knadh
Copy link
Owner

knadh commented Aug 30, 2020

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

@clementauger
Copy link
Contributor Author

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
Copy link
Owner

knadh commented Aug 30, 2020

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
Copy link
Contributor Author

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
Copy link
Owner

knadh commented Aug 30, 2020

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
Copy link
Contributor Author

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
Copy link
Owner

knadh commented Aug 30, 2020

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

No. Go will soon get file embedding support in its core. See golang/go#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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

knadh commented Sep 2, 2020

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
Copy link
Contributor Author

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.

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.

2 participants