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

[Bug] Fix path to built-in Vizro assets #358

Merged
merged 6 commits into from
Mar 11, 2024
Merged

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Mar 11, 2024

Description

There was a subtle bug where we were using routes_pathname_prefix somewhere we should have used requests_pathname_prefix. What this meant was that in some deployments our built-in Vizro js and css would not be found.

  • requests_pathname_prefix is "is a local URL prefix for file requests. Must end with routes_pathname_prefix" (from Dash docs). "the prefix for the AJAX calls that originate from the client (the web browser)" (from comment in Dash source code). This is what's used when injecting flask server into Dash as in Host on a route of existing Flask app
  • routes_pathname_prefix is "a local URL prefix for JSON requests. Must start and end with '/'" (from Dash docs). "the prefix for the API routes on the backend (this flask server)" (from comment in Dash source code). This is what's used when mounting Dash onto a server using middleware (could be WSGI like flask or ASGI like FastAPI that uses WSGIMiddleware) as in DispatcherMiddleware
  • setting url_base_pathname = x is equivalent to setting both requests_pathname_prefix = x and routes_pathname_prefix = x

Previously things worked ok when requests_pathname_prefix == routes_pathname_prefix but not otherwise. Now it works ok - tested on:

  • injecting flask server into Vizro
  • mounting Vizro into Flask app
  • mounting Vizro into FastAPI app

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@antonymilne antonymilne changed the title Bug/external assets [Bug] Fix path to external assets Mar 11, 2024
@antonymilne antonymilne changed the title [Bug] Fix path to external assets [Bug] Fix path to built-in Vizro assets Mar 11, 2024
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@antonymilne antonymilne enabled auto-merge (squash) March 11, 2024 19:55
@antonymilne antonymilne merged commit b1e5a2b into main Mar 11, 2024
33 checks passed
@antonymilne antonymilne deleted the bug/external-assets branch March 11, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Report 🐛 Issue contains a bug report
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants