Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support for config-aware relative paths #1073
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
Changes from 4 commits
0af759c
249d305
7e620a9
6b3978d
02a4295
868e93a
57cf39b
dd790ab
86b7665
a0b5f1f
20dd95f
0c0af34
685cd69
c11aabe
31854eb
5879961
46af325
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I suppose a user could code complete their way into

dash.dash
, instead of usingdash.Dash
:But we've got a bunch of other extraneous stuff in there already like
dash_renderer
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:
path
and not a full URL although folks might think that "path" is corresponding to the file system path.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 fromassets_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
intoget_asset_url
if we're expecting windows users to directly useget_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 insidedcc.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:Or, rather, the trailing slashes case:
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 868e93aThe documented example should actually be
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 haveget_relative_path
work add a leading/
):At which point you could even avoid having to reference all the pages individually, since
page_name
would be fully normalized: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 oftrim_path
andrelative_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 ofget_relative_path
in 20dd95fThere 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 arelative-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 ofthis-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)
, becausehistory.pushState
will resolve relative paths based on the current url, butlocation.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