-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Allow overriding base path for UI/API routes; rm --query.prefix #748
Conversation
@@ -1,6 +1,7 @@ | |||
<!DOCTYPE html> | |||
<html lang="en"> | |||
<meta charset="UTF-8"> | |||
<base href="/" data-inject-target="BASE_URL"/> |
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.
@tiffon this models the compiled index.html
from UI assets.
Q: is it possible that babel/webpack would reorder the href
and data-inject-target
attributes? Because if it does, the search&replace in the query service won't work
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.
@yurishkuro It should not change the order of the attributes.
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.
LGTM, but would be awesome to have the user notified when the HTML on disk is not what is being served to clients. Also, it would perhaps be worth adding some piece of documentation about this (when to use/why/how)
options.BasePath = "/" | ||
} | ||
if options.BasePath != "/" { | ||
if !strings.HasPrefix(options.BasePath, "/") || strings.HasSuffix(options.BasePath, "/") { |
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 it possible to add a log statement notifying that what people see in the HTML is not what will be served to clients?
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.
could you elaborate why you think this would be useful? We're modifying the internal file of the app, not something that users provide.
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.
Mainly because it's deviating from what is "expected": if I had a problem with the static resources (like the original problem this PR is solving) and were to check the HTML, I would wonder why it's not the same as what my browser is seeing. A log entry at debug
level would suffice, IMO.
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.
well, still not convinced why someone needs to know what happens internally, e.g. we also override the config, and in both cases we only do that when requested via cli arguments.
Let's do it separately, because the constructor doesn't take a logger and the unit tests explicitly verify that nothing is logged, so it's not as trivial a change as it sounds for a feature that seems rather marginal.
Docs would be updated in a separate PR in the docs repo. |
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
90f4635
to
7a019df
Compare
Resolves #745 Provide a means to define a path-prefix for the UI
Resolves #746 Remove --query.prefix option