Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Assets helpers are not CDN friendly #921

Open
TankTheFrank opened this issue Feb 15, 2018 · 11 comments
Open

Assets helpers are not CDN friendly #921

TankTheFrank opened this issue Feb 15, 2018 · 11 comments
Assignees
Labels
enhancement New feature or request s: hold shouldn't be closed, should wait for further work.
Milestone

Comments

@TankTheFrank
Copy link

First

return filepath.ToSlash(filepath.Join("/assets", filePath))

prepends "/assets" (hardcoded) to the urls from manifest.json making the assets template helpers ( javascriptTag, stylesheetTag, etc) unusable with assets served from a separate domain.

Second, I could not find a straight-forward way of building a binary that packs the "must have" files (templates, migrations, manifest.json) while leaving the bulk of assets from the "asset" directory (css, js, images) unpacked.

@paganotoni
Copy link
Member

Hey @TankTheFrank, may buffalo build -e work for you? it generates a separate archive with the assets.

Also, which are your suggestions to make buffalo CDN friendly?

@TankTheFrank
Copy link
Author

I forgot to add the fix proposal for the hardcoded path - add "/assets" as "basePath" in webpacker manifest plugin for the default template and remove it as a hardcoded prefix.

@TankTheFrank
Copy link
Author

I could be wrong but doesn't -e also adds the packed assets in the binary beside creating the archive? This increases the binary size significantly (esoecially with image assets) and pointless memory at run time when assets are not served from it.

I tried a mix of -k -e and other options and I ended up either without migrations (running migrate did nothing) or with a large file size. The "solution" that seemed to work was to generate the assets, delete CSS, images, js etc from the public dir and build again with -k (skip asset generation). Hence the "no easy way" part.

@paganotoni
Copy link
Member

Got it, I don't think its on purpose that -e keeps the assets in the binary as well (after extracting them on the separate archive) so, to me this looks like a bug, (unless @stanislas-m and/or @markbates correct me) would you want to work on a PR for this ? (PR's are always welcome).

Also, thanks for the suggestion on the /assets prefix, let's work on that one as well.

@markbates
Copy link
Member

If -e or -k is still packing assets into the binary, that's definitely a bug.

It's mostly an issue with these two lines:

https://github.com/gobuffalo/buffalo/blob/master/buffalo/cmd/build/assets.go#L24
https://github.com/gobuffalo/buffalo/blob/master/buffalo/cmd/build/assets.go#L28

A PR to fix it would be fantastic.

@TankTheFrank
Copy link
Author

For the prefix this should do it:

generators/assets/webpack/templates/webpack.config.js.tmpl

 new ManifestPlugin({
+      publicPath: '/assets/',
       fileName: "manifest.json"
     })

render/template_helpers.go b/render/template_helpers.go

-       return filepath.ToSlash(filepath.Join("/assets", filePath))
+       return filepath.ToSlash(filePath)

Basically the "/assets/" prefix is added to manifest.json entries when it's generated instead of being forced at runtime.

@stanislas-m stanislas-m changed the title not CDN friendly Assets helpers are not CDN friendly Feb 16, 2018
@stanislas-m stanislas-m added the enhancement New feature or request label Feb 16, 2018
@stanislas-m
Copy link
Member

I just tested the -e flag and on the development version at least, it works well: the assets are not packaged in the binary.

@paganotoni
Copy link
Member

I took a look at it and that's the same I saw.

stanislas-m added a commit that referenced this issue Apr 14, 2018
* Fix #921: use webpack publicPath, instead of hard-coded "/assets/".
@stanislas-m stanislas-m added the s: in progress Someone is working on this label Apr 16, 2018
@stanislas-m stanislas-m self-assigned this Apr 16, 2018
@H3rby7
Copy link

H3rby7 commented Oct 30, 2019

@stanislas-m was there any progress on this issue? Is there anything I could do to fix it?

Apart from the CDN friendlyness this issue also prevents asset helpers being useful if the web-app should not run under the "/" path, but a different path, because it is behind some sort of ingress management.

(see: https://gophers.slack.com/archives/C3MSAFD40/p1572282815126800) - where I also talked to @paganotoni

@stanislas-m
Copy link
Member

@H3rby7 The fix proposed in #1030 should work, but we wanted to not break stuff either by providing a fallback, or a fix in the buffalo fix command.

@ArmandBENETEAU
Copy link

ArmandBENETEAU commented Jul 6, 2020

Hi all,

I am currently trying to extract the assets from the built binary like @TankTheFrank talked about in his first message.

And it seems that I have the same problem: when I pass the "-e" option to the "buffalo build" command line, the assets are correctly zipped in a separate ".zip" file, but they seems to keep being in the binary (basically, the size of the binary doesn't change).

It has to be noticed that I am in "production" mode.

Do I miss something?

Thanks in advance for your help!

@sio4 sio4 added s: hold shouldn't be closed, should wait for further work. and removed s: in progress Someone is working on this labels Sep 26, 2022
@sio4 sio4 added this to the Backlog milestone Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request s: hold shouldn't be closed, should wait for further work.
Projects
None yet
Development

No branches or pull requests

7 participants