TandoorRecipes / recipes

Application for managing recipes, planning meals, building shopping lists and much much more!
https://docs.tandoor.dev
Other
5.61k stars 597 forks source link

0.17.2 Ignores STATIC_URL when generating static files #972

Closed eljef closed 3 years ago

eljef commented 3 years ago

Version

Version: 0.17.2

Bug description

When using STATIC_URL that is not default, in combination with 0.17.2, files are not being prefixed with the STATIC_URL.

My STATIC_URL is set to "/tandoor/static". When loading Tandoor, loading static assets will fail when navigating away from the first logged in page.

v0.17.2

GET https://<domain>/static/vue/js/chunk-vendors.js 404
GET https://<domain>/static/vue/css/chunk-vendors.css 404
GET https://<domain>/static/vue/js/recipe_view.js 404
GET https://<domain>/static/vue/css/recipe_view.css 404

v0.17.1

GET https://<domain>/tandoor/static/vue/css/chunk-vendors.eeb255433fa0.css 200
GET https://<domain>/tandoor/static/vue/css/recipe_view.cb0bf1a69861.css 200

When looking at the generated static files for 0.17.2, all urls are hard-coded with "/static/vue/...". When inspecting generated static files for 0.17.1, all urls correctly prefixed. ie "/tandoor/static/..."

I've also verified that SCRIPT_NAME and JS_REVERSE_SCRIPT_PREFIX are set correctly. This has no effect on the outcome.

eljef commented 3 years ago

I don't know if commit df31f7388e304a5f7bf4269bdc5c3bbba33fd2a6 has anything to do with it, but the addition of /static/vue as a hardset path in vue/vue.config.js might have some effect?

smilerz commented 3 years ago

Do you have the SCRIPT_NAME header set in the nginx config?

proxy_set_header X-Script-Name /xxxx

eljef commented 3 years ago

Yes I do. It is set to "/tandoor". I tried removing it as well, with no effect.

Removing it in 0.17.1 results in Error whilst registering service worker DOMException: The operation is insecure. in the console.

smilerz commented 3 years ago

I have tandoor running as a subfolder on nginx with a conf that looks like this.

https://github.com/vabene1111/recipes/blob/ad1ad1a9e59aec75b88bf76a446cd45baa848bb0/nginx/conf.d/recipe.subfolder.conf.template

eljef commented 3 years ago

My nginx config is almost identical. And it works flawlessly with 0.17.1.

Upgrading to 0.17.2 results in the bug as posted.

vabene1111 commented 3 years ago

0.17.2 should not have changed anything with how static files are served or where they are stored but i will look into this as soon as i can, for now i recommend staying on 0.17.1

stewartadam commented 3 years ago

Same issue here after updating to 0.17.2 (with Apache as reverse proxy). Static URL generation broke on the recipe pages, but works for the search/landing page.

vabene1111 commented 3 years ago

i do see how https://github.com/vabene1111/recipes/commit/df31f7388e304a5f7bf4269bdc5c3bbba33fd2a6 might have impacted how the client determines the url for the API files (this was the reason i changed it in the first place) but i am confused on why this works for @smilerz and not for the other setups if they are basically the same.

That said i am not sure how in general it would be possible to tell webpack to load from a url at runtime as the path is set at compile time.

@smilerz you are running 0.17.2 with the change i made to the vue.config.ts correct or are you running your fork ?

i will try to read a little into how the webpack configuration works with user configurable sub paths

smilerz commented 3 years ago

@smilerz you are running 0.17.2 with the change i made to the vue.config.ts correct or are you running your fork ?

I haven't updated my instance with 0.17.2 yet. I'll take a look when I get the shopping list useable, probably later this week.

vabene1111 commented 3 years ago

ok so its likely you will have the same issue.

I never really looked into how sub path hosting is working as sub domains are bascially free everywhere and i never saw a reason why sub paths would be better than a sub domain so i am lacking quite a bit of knowledge here. I would have imagined that the web server treats a sub path basically as if it was the domain/host part and thus the application still runs under the base path /static/vue (from its sub path included "host").

This does not seem to be the case so we will need to find out how to, similar to the open api, set the base path dynamically when running the application.

vabene1111 commented 3 years ago

Ok so i have so far understood (and writing this down for future me who forgets about it) that the previous base path configuration ('') meant that web pack determines its path automatically (as mentioned in this issue https://github.com/webpack-contrib/file-loader/issues/46#issuecomment-264215576) but the documentation also says that you can specifically set automatic behavior by using 'auto' as the public path value https://webpack.js.org/guides/public-path/

The reason i changed the public path in the first place was that i introduced a piece of code that dynamically loads other javascript components. This promted webpack to build bundles for every component that i could theoretically load at this path. This worked well on my development server (which makes sense as that is http://localhost:8080/ with all scripts at the base path) but did not work once the source files were build.

I will test if setting auto will work better or if there is any way to dynamically set __webpack_public_path__ from maybe the local storage BASE_PATH variable smilerz introduced with the last update.

vabene1111 commented 3 years ago

ok so it appears the webpack public path and vue public path are different things https://cli.vuejs.org/config/#publicpath

further testing is ongoing sorry for the long wait

vabene1111 commented 3 years ago

Ok so using a relative path does not work as it is relative not to the application root but the page the user is currently on

GET https://<domain>/search/static/vue/js/chunk-vendors.js 404

I dont have any more time right now but this basically means we have to find out the equivalent of __webpack_public_path__ for vue cli and how to set this at runtime so that if a different base path is used the frontend can still dynamically find out where it needs to make its requests.

vabene1111 commented 3 years ago

This likely does the trick by using the local storage base path or something derived from it https://stackoverflow.com/a/55955479/6478110

eljef commented 3 years ago

I never really looked into how sub path hosting is working as sub domains are bascially free everywhere and i never saw a reason why sub paths would be better than a sub domain

This is a privacy thing really. While sub-domains are free, they are wide-open in DNS records. Simply digging a domain can reveal what services are being hosted somewhere. On the inverse, subfolders are only published where a hoster wants to have them published. If that is behind a password protected front-end, no-one knows the services that are being hosted besides the people that can login. It's somewhat paranoid, but it makes some people sleep better at night.

That being said, sed filtering the path on the static files does fix the issue, so I would think having __webpack_public_path__ correctly set will fix issues accordingly.

smilerz commented 3 years ago

I'm starting to dig into this a bit now - but probably wouldn't see this in my setup as I have static and media both set as subfolders (i.e. not sub-subfolders like /recipes/static).

quick question for those of you seeing this - do you have these ENV settings modified?

# If staticfiles are stored at a different location uncomment and change accordingly
# STATIC_URL=/static/

# If mediafiles are stored at a different location uncomment and change accordingly
# MEDIA_URL=/media/

and can one of you share the full nginx config that includes the /static and /media settings?

eljef commented 3 years ago

Here are my current ENV settings for URLs that works correctly with 0.17.1

STATIC_URL=https://<domain>/tandoor/static/
MEDIA_URL=https://<domain>/tandoor/media/

I have also tested with the following and it works

STATIC_URL=/tandoor/static/
MEDIA_URL=/tandoor/media/

This is the tandoor.conf that I include into my nginx config. This currently works flawlessly in 0.17.1.

    location /tandoor/static {
        root /srv/http/home;
        client_max_body_size 128M;
    }
    location /tandoor/media {
        root /srv/http/home;
        client_max_body_size 128M;
    }

    location /tandoor {
        include             /etc/nginx/conf.d/00-security-headers.conf;
        include             /etc/nginx/conf.d/00-fp-default.conf;
        include             /etc/nginx/conf.d/00-csp-default.conf;

        proxy_pass          http://172.17.0.1:8089;

        proxy_cookie_path   /    /tandoor;

        proxy_set_header    Host              $host;
        proxy_set_header    X-Forwarded-Proto "https";
        proxy_set_header    X-Forwarded-For   $proxy_add_x_forwarded_for;
        proxy_set_header    X-Script-Name     /tandoor;

        client_max_body_size 128M;
    }

I use docker-compose to bring this up on a closed bridge, mapping port 8089 to the container. This is all internal docker networking though, and does not affect the external URL that is meant for tandoor.

smilerz commented 3 years ago

thanks @eljef

@vabene1111 looking at this issue it looks like the webpack will either use STATIC_URL or publicpath when collectstatic runs. What are you trying to do that publicpath was necessary?

If it is necessary to set publicpath why not just use the environment variable to catch STATIC_URL and build the path at compile time? (see here)
This would theoretically non-breaking and the intuitive answer anyway as the webpacks are expected to be at STATIC_URL and its already configured.

vabene1111 commented 3 years ago

First of all @eljef thanks for the answer on sub paths, i have never thought about that but it makes perfect sense, thank you!

Second: The reason i needed to set the public path was that (and i am explaining the technicallities for @smilerz mostly) i added the generic component rendering to the ModelForm (what i send you on dicord the other day). Because at compile time webpack does not know which bundles will be loaded it builds one bundle for each file that could theoretically be loaded and then at runtime decides which on it is. This worked well on my local dev server but did not on my dockerized deployment (which is weird on its own since running build on a local setup should work just as build on the deployment works even with dev server). I fixed this issue by setting publicPath but apparently this breaks sub path configurations.

As far as i understand static_url is not used because webpack needs to compile those paths into the javascript code at compile time so webpack bundle tracker has no chance in rendering the correct bundle (the "sub bundles" if we want to call them that are not rendered via the django template and dont even show up in the bundle tracker json).

Next thing i am going to try which should work is setting __webpack_public_path__ properly on runtime

vabene1111 commented 3 years ago

ok so i have been playing around with webpack_public_path with little success but i found this post detailing a little more about the problem

https://stackoverflow.com/a/65924415/6478110

vabene1111 commented 3 years ago

ok so for some reason i could reproduce the issue on my local setup now (no idea why it did not work before)

setting __webpack_public_path__ like this before the app is mounted with STATIC_URL being a django determined location for the static paths seems to work locally, going to test on my test server now but this should then also work for sub path deployments.

export default __webpack_public_path__ = localStorage.STATIC_URL + 'vue/' // eslint-disable-line
vabene1111 commented 3 years ago

ok so everything works on my test server as well (altough no sub path configuration there), i updated the beta image so you should be able to test if this works now

eljef commented 3 years ago

@vabene1111 The beta image works. Do you have a commit that fixes this?

eljef commented 3 years ago

I just saw 6949e17a33a781a0b69fdb8fe5cca239c0f16fe8 Seems to be working as expected now. Thanks for your work.

vabene1111 commented 3 years ago

Awesome, you can also use the release image, we updated yesterday.

Thanks for the help debugging and sorry for the inconvenience.