search

stewartpark / Flask-JSGlue

Flask-JSGlue helps hook up your Flask application nicely with the front end.
80 stars 40 forks source link

Fix URL generated by JSGlue.include() #4

Closed jonafato closed 9 years ago

jonafato commented 9 years ago

Currently, the URL generated by JSGlue.include() in the script tag is static and may be incorrect depending on the application configuration. This change uses url_for instead of the static value to handle the case where the Flask application is not served at the site root. Fixes #2.

stewartpark commented 9 years ago

Can you explain why this would be the right way of doing this?

I've been trying to think of an elegant way to do this, but the only thing I could come up was using the Flask's configuration system (i.e. app.config)

Won't this break if there's no serve_js route?

Please enlighten me If I'm missing something :) Thanks for the input, though!

jonafato commented 9 years ago

You're correct that it would break if there's no serve_js route, but that's added to the app in init_app. If init_app is never called, it doesn't make sense to try to use the JS, since the route that serves the helper would end up being a 404. To handle that breakage, a check could be added to see if the extension has been registered on the application. If it hasn't, a custom exception could be raised (though this just shifts from one exception to another in that case). The current behavior, however, is a silent failure on the back end and JavaScript errors on the front end. An exception and an early failure would be preferred and easier to track down and fix in my opinion.

The specific issue this fixes (and the reason why url_for is the correct way to do this) is the case where the application is not at the root of the domain (e.g. being served at a path other than / behind nginx, combined with another wsgi application behind werkzeug's DispatcherMiddleware). In this case, using JSGLUE_JS_PATH is not enough for Flask to find the route. url_for handles this by using additional information from the wsgi environment to build out the full, correct URL to the serve_jsendpoint. (See nvie/rq-dashboard#68 for how this fixed a similar issue.)

stewartpark commented 9 years ago

Oh, I see. Now it's all clear. You are absolutely correct. Thanks for the great explanation! Going to be merged!