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: Viz not working with proxy URLs #941

Merged
merged 14 commits into from
Jul 1, 2022
Merged

Conversation

tynandebold
Copy link
Member

@tynandebold tynandebold commented Jun 26, 2022

Description

Fixes #916

Development notes

If a user runs Viz with a proxied URL, like you may try to do as described in this issue, the application would load without critical information, and sometimes no information at all. The reason being is that the URL wouldn't pass our hard-coded route match.

In this PR, we'll now allow a dynamic pathname to be recognized and matched, thus allowing the flowchart or experiment-tracking pages to render correctly.

Lastly, we'll also sanitize the URL and remove any duplicative experiment-tracking strings from appearing in the pathname. We need to guard against this because, again, if a user has a proxy URL or a base URL that isn't /, there will be conflicts. The use case: a user runs Viz with jupyter-sever-proxy, which adds /kedro_viz/ to the pathname. So Viz loads, then we navigate to the experiment-tracking page, which does load correctly. Yet if the page is refreshed, we'd receive an incorrect result. If we didn't remove experiment-tracking from the pathname, we'd get something like /kedro_viz/experiment-trackingexperiment-tracking, and that's not desirable.

QA notes

Run Viz via jupyter-sever-proxy by doing the following:

  • Checkout the branch
  • cd in the demo-project folder
  • pip install jupyter-sever-proxy
  • Run kedro viz
  • Click the Kedro Viz icon that appears

All functionality within Viz should function as it should.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

This is great! I will test it out later today.

Let's leave out all the Python stuff and icon for now since it shouldn't be needed just to fix the proxy bug. It needs a bit more thought (the stuff I did was just a quick POC) and would be done as part of a modified #910.

src/components/icons/logo.js Outdated Show resolved Hide resolved
package/setup.py Outdated Show resolved Hide resolved
@tynandebold tynandebold requested a review from antonymilne June 27, 2022 09:21
Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
@tynandebold tynandebold requested a review from yetudada as a code owner June 27, 2022 09:27
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

This is great!! But... it doesn't actually seem to work for me 🤔 I'm not sure why because when I check the window.location it seems like it should. Probably I've just not set it up correctly locally. Let me show you on Wednesday and hopefully we can figure it out.

I'm also wondering:

  • should we make this even more generic to include search and hash as well as pathname? I think this actually might be needed in real scenarios with jupyter server proxy, not just a theoretical possibility. Seems like it's possible to pass a location object to these tags rather than needing to reconstruct the host + pathname + search + hash string
  • I don't quite get where the / should be still 😅 I'm sure what you've done is right but I need to think through it again with a fresh head to understand

src/apollo/config.js Outdated Show resolved Hide resolved
src/apollo/config.js Outdated Show resolved Hide resolved
src/components/global-toolbar/global-toolbar.js Outdated Show resolved Hide resolved
@tynandebold
Copy link
Member Author

This is great!! But... it doesn't actually seem to work for me 🤔 I'm not sure why because when I check the window.location it seems like it should. Probably I've just not set it up correctly locally. Let me show you on Wednesday and hopefully we can figure it out.

I'm also wondering:

  • should we make this even more generic to include search and hash as well as pathname? I think this actually might be needed in real scenarios with jupyter server proxy, not just a theoretical possibility. Seems like it's possible to pass a location object to these tags rather than needing to reconstruct the host + pathname + search + hash string
  • I don't quite get where the / should be still 😅 I'm sure what you've done is right but I need to think through it again with a fresh head to understand

Oh no, ok, let's sync up today.

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Awesome stuff. Super excited to get this fixed and what it means we can hopefully do next 💥

It would be nice if a future iteration also passed the search and hash (looks like Link at least could handle this easily). And also if we can eventually figure out how to remove the manual path-sanitisation workaround.

src/components/wrapper/wrapper.js Outdated Show resolved Hide resolved
src/components/wrapper/wrapper.js Outdated Show resolved Hide resolved
@tynandebold tynandebold removed the request for review from yetudada July 1, 2022 10:45
@tynandebold tynandebold merged commit af4412c into main Jul 1, 2022
@tynandebold tynandebold deleted the hack/jupyter-server-proxy branch July 1, 2022 10:54
@tynandebold tynandebold mentioned this pull request Jul 1, 2022
5 tasks
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.

Kedro viz with proxy does not show additional information
2 participants