-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support for config-aware relative paths #1073
Conversation
gotta love test driven development
tests/unit/test_configs.py
Outdated
("/my-dash-app", "/page-1/sub-page-1", "/my-dash-app/page-1/sub-page-1"), | ||
|
||
("/", "relative-page-1", "relative-page-1"), | ||
("/my-dash-app", "relative-page-1", "/my-dash-apprelative-page-1"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These relative URLs are ambiguous in this scenario as we don't know what page we're on.
For example, if we were on page /my-dash-app/this-page
and then provided a relative-page-1
, the resolved URL should be /my-dash-app/this-page/relative-page-1
but we can't provide that to the front end because we are unaware of this-page
.
So, I thought we'd just keep it a simple prefix without any additional logic.
Relative URLs aren't documented in Dash nor used very much anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... I almost wonder if we should prohibit relative paths. It currently has a bug, I believe, if you use dcc.Link(refresh=True)
, because history.pushState
will resolve relative paths based on the current url, but location.pathname = '...'
will just prepend a /
if you don't provide one, ie treat it as an absolute path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds good. I added an exception in a0b5f1f
@@ -1565,6 +1566,28 @@ def get_asset_url(self, path): | |||
|
|||
return asset | |||
|
|||
def get_relative_path(self, path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming options:
dcc.Link(href=app.get_relative_path('/page-2'))
dcc.Link(href=app.get_relative_url('/page-2'))
dcc.Link(href=app.get_rel_path('/page-2'))
dcc.Link(href=app.get_rel_url('/page-2'))
dcc.Link(href=app.relative_path('/page-2'))
dcc.Link(href=app.relative_url('/page-2'))
dcc.Link(href=app.rel_path('/page-2'))
dcc.Link(href=app.rel_url('/page-2'))
dcc.Link(href=app.relpath('/page-2'))
dcc.Link(href=app.relurl('/page-2'))
Notes:
- It is technically a
path
and not a full URL although folks might think that "path" is corresponding to the file system path. - We use
get_
in other places likeget_asset_url
. Do we keep it for consistency? I'd prefer to moveget_asset_url
usage over to this function, so folks just need to learn e.g.app.relpath('/assets/logo.png')
andapp.relpath('/page-2')
I think I prefer app.relative_path('/page-2')
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_asset_url
is still nice in order to decouple the usage from assets_url_path
. Also in that case it is a file path (that you pass in - though it occurs to me it's a non-Windows path, we may want to move or copy the .replace("\\", "/")
that we currently have in _on_assets_change
into get_asset_url
if we're expecting windows users to directly use get_asset_url
) - so it seems like our nomenclature is distinguishing file path from (partial) url, rather than url path from full url.
The other thing I'm wondering is whether we need the reverse operation, stripping off requests_pathname_prefix
, for use inside dcc.Location
callbacks? If you think that would be useful, then we should name these two methods for clarity in tandem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For use in dcc.Location
, I'm now thinking that we could include this function in the callback url matching logic like:
@app.callback(...)
def display_content(path):
if (path is None or
app.get_relative_path(path) == app.get_relative_path('/')):
return chapters.home
elif app.get_relative_path(path) == app.get_relative_path('/page-1')
return chapters.page_1
elif app.get_relative_path(path) == app.get_relative_path('/page-2')
return chapters.page_2
Or, rather, the trailing slashes case:
@app.callback(...)
def display_content(path):
if path is None or app.get_relative_path(path) == app.get_relative_path("/"):
return chapters.home
elif app.get_relative_path(path) in [
app.get_relative_path("/page-1"),
app.get_relative_path("/page-1/"),
]:
return chapters.page_1
elif app.get_relative_path(path) in [
app.get_relative_path("/page-2"),
app.get_relative_path("/page-2/"),
]:
return chapters.page_2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That last example actually exposes another missing case, ''
, fixed in 868e93a
The documented example should actually be
@app.callback(...)
def display_content(path):
if path is None or app.get_relative_path(path) in [
app.get_relative_path(""),
app.get_relative_path("/"),
]:
return chapters.home
elif app.get_relative_path(path) in [
app.get_relative_path("/page-1"),
app.get_relative_path("/page-1/"),
]:
return chapters.page_1
elif app.get_relative_path(path) in [
app.get_relative_path("/page-2"),
app.get_relative_path("/page-2/"),
]:
return chapters.page_2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose, but folks may appreciate simplifying it (including stripping the trailing /
and maybe even the leading one, if we were to have get_relative_path
work add a leading /
):
@app.callback(...)
def display_content(path):
page_name = app.trim_path(path)
if not page_name: # None or ''
return chapters.home
elif page_name == 'page-1':
return chapters.page_1
if page_name == "page-2":
return chapters.page_2
At which point you could even avoid having to reference all the pages individually, since page_name
would be fully normalized:
@app.callback(...)
def display_content(path):
page_name = app.trim_path(path)
if not page_name: # None or ''
return chapters.home
return getattr(chapters, page_name.replace("-", "_"))
# or make chapters be a dict that maps without the replacement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's way better! I'll work on a trim_path
method. There should definitely be symmetry between the naming of trim_path
and relative_path
, whatever we decide on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I added strip_relative_path
which would match the naming of get_relative_path
in 20dd95f
@@ -36,6 +36,7 @@ | |||
from . import _watch | |||
from ._utils import get_asset_path as _get_asset_path | |||
from ._utils import create_callback_id as _create_callback_id | |||
from ._utils import get_relative_path as _get_relative_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this pattern (import x as _x
) serve a purpose, given that we re-export the symbols we're interested in from __init__.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dash/_utils.py
Outdated
"You supplied: {}".format(path) | ||
) | ||
if requests_pathname != '/': | ||
path = path.replace(requests_pathname, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason we'd get here with path
not starting with requests_pathname
? Should it be an error if we do?
And regardless of that, we should only remove requests_pathname
at the beginning of path
, right? I mean, you could imagine an app hosted at app/
with a path app/sub-app/
or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the dcc.Location.pathname
usage, I can't imagine a case where we'd get a path that doesn't start with requests_pathname
. We could error out.
Good point re beginning of path. The only exception is if the prefix is /my-dash-app/
but then the app might be served under /my-dash-app
and dcc.Location
might pass /my-dash-app
through. In Dash Enterprise, we redirect to the trailing slash version (e.g. https://dash-demo.plotly.host/ddk-oil-and-gas-demo redirects to https://dash-demo.plotly.host/ddk-oil-and-gas-demo/) but I could imagine that that redirect logic could inadvertently change in the future sometime.
I updated the logic and added a test case for this in 685cd69
Co-Authored-By: alexcjohnson <johnson.alex.c@gmail.com>
and paths that look like request_pathname_prefix but don’t have the trailing slash
…e-paths-2 Conflicts: dash/_utils.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog needs updating for the new name & second method, but after that it looks ready to go!
💃 love it!
Same as #1070 but rebased off of
dev
Fixes https://github.com/plotly/dash-core/issues/78