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

Fix assets blueprint path management #547

Merged
merged 7 commits into from
Jan 23, 2019
Merged

Fix assets blueprint path management #547

merged 7 commits into from
Jan 23, 2019

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Jan 20, 2019

Fixed

  • Asset blueprint takes routes prefix into it's static path.
  • Asset url path no longer strip routes from requests.

Changed

  • assets_folder argument now default to 'assets'
  • The assets folder is now always relative to the given root path of name argument, the default of __main__ will get the cwd.
  • No longer coerce the name argument from the server if the server argument is provided.

Fixes #529

@T4rk1n T4rk1n requested a review from ned2 January 20, 2019 01:04
dash/dash.py Outdated
assets_url_path='/assets',
assets_ignore='',
assets_blueprint_name='assets',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How important is this to be a user-configurable param? If we made the initial value dash-assets (which would still be prefixed by custom route values -- which was a good move btw) then it seems unlikely that a user would need to modify this.

Just thinking about slowing the rate of kwarg growth for the constructor where possible, as it will get a but unwieldy at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point let's remove that, there's already much clutter in the kwargs. 👌

Copy link
Contributor

@ned2 ned2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, the fix to adding the routes prefix to the assets URL setting and getting looks good!

With the fixes to add better support for multi-route blueprints, I'm wondering if we could improve this some more.

One scenario that I could readily see happening when trying to support multiple Dash apps alongside each other using the same Flask server:

server.py
app1/app1.py
app2/assets/app1.css
app2/app2.py
app2/assets/app2.css

Where server.py defines a Flask instance and app1.py and app2.py both define Dash instances that pass in the Flask instance from the directory above.

app1's Dash instance might look like this:

app = dash.Dash(
    server=server,
    requests_pathname_prefix='/my-app1/',
    routes_pathname_prefix='/my-app1/',
)

The assets for this app won't actually work however, as the default value of assets_folder is derived from the cwd of the Flask instance. So it will automatically be set to assets (ie at the same level as server.py. So instead the user will have to do:

app = dash.Dash(
    server=server,
    assets_folder="app1/assets",
    requests_pathname_prefix='/my-app1/',
    routes_pathname_prefix='/my-app1/',
)

There would be much less cognitive friction if the correct value of the assets path could be inferred as being relative to the Dash instance rather than the Flask instance. I guess this situation could be more generally described as when a Dash instance isn't found within the path that server.name resolves to. Which, now that I think about it, might actually be a lot harder to make happen, so maybe this is a discussion for another time...

@ned2
Copy link
Contributor

ned2 commented Jan 20, 2019

Here's the example app I was using to check this was working and find that issue with the static_folder value of the Blueplrint: https://github.com/ned2/dash-embed-recipes/tree/master/flask_multi

It's has the same layout as in the example I mentioned above.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Jan 21, 2019

@ned2 Your examples are missing the __name__ , but still it won't work because when we provide the server, the name is taken from the server instead so it won't be relative to the app but instead will be relative to where the server was instantiated.

Should we remove that or improve the logic here ?

name = name if server is None else server.name

@ned2
Copy link
Contributor

ned2 commented Jan 22, 2019

@ned2 Your examples are missing the __name__ , but still it won't work because when we provide the server, the name is taken from the server instead so it won't be relative to the app but instead will be relative to where the server was instantiated.

Should we remove that or improve the logic here ?

dash/dash/dash.py

Line 105 in e72f483

name = name if server is None else server.name

Yeah, I didn't include the name argument precisely because I knew that it would be changed to be the value of server.name.

yep, so a potential change could be to remove that line overriding name, then flask.helpers.get_root_path(name) would resolve to the path the file with the Dash instance is found in, but server.name would still be whatever the original Flask instance was set to.

Are there any other places that would be affected by decoupling the Dash name param from the Flask name param? I can't see any scanning through dash.py

(also, if we did this, should it be in a separate PR?)

@ned2
Copy link
Contributor

ned2 commented Jan 22, 2019

I just tested the effect of removing that line that overwrites the name variable with server.name on my example when both Dash instances are updated to pass the name parameter. Looks like it's all working exactly as it should now!

If you think this won't have any other unwanted repercussions, then I'm happy to make that change. I'm also happy with how everything else is looking.

💃

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Jan 23, 2019

Thanks @ned2, let's remove the line, I don't think it's useful.

@T4rk1n T4rk1n merged commit 534d285 into master Jan 23, 2019
@T4rk1n T4rk1n deleted the fix-asset-paths branch January 23, 2019 16:38
alexcjohnson added a commit to plotly/dash-core-components that referenced this pull request Jan 24, 2019
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.

Don't strip asset paths
2 participants