feat: Relative links by mofojed · Pull Request #1204 · deephaven/web-client-ui

mofojed · 2023-04-04T20:30:15Z

Notebooks work
Links between notebooks work
Deeplinking notebooks works in dev mode with proxy
- Does not work in production - would need server to correctly proxy URLs and write the correct document base
Embed-grid works
Embed-chart works
Plugins work
Fixes Relative UI links #1070
Unit tests and e2e tests pass
Tested in both a dev build, and production build
Tested following steps in Relative UI links #1070
Tested in DnD, ensured the correct paths were being loaded

codecov · 2023-04-04T20:38:35Z

Codecov Report

Merging #1204 (d47595f) into main (769d753) will decrease coverage by 0.02%.
The diff coverage is 18.18%.

@@            Coverage Diff             @@
##             main    #1204      +/-   ##
==========================================
- Coverage   45.57%   45.56%   -0.02%     
==========================================
  Files         484      484              
  Lines       34077    34081       +4     
  Branches     8532     8532              
==========================================
- Hits        15531    15528       -3     
- Misses      18495    18502       +7     
  Partials       51       51

Flag	Coverage Δ
unit	`45.56% <18.18%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/app-utils/src/components/AppBootstrap.tsx	`87.50% <ø> (-1.39%)`	⬇️
packages/code-studio/src/index.tsx	`0.00% <0.00%> (ø)`
packages/code-studio/src/main/AppInit.tsx	`0.00% <0.00%> (ø)`
packages/code-studio/src/main/AppMainContainer.tsx	`34.94% <0.00%> (-0.13%)`	⬇️
packages/code-studio/src/main/AppRouter.tsx	`0.00% <0.00%> (ø)`
packages/code-studio/src/styleguide/index.tsx	`0.00% <0.00%> (ø)`
...ackages/app-utils/src/components/AuthBootstrap.tsx	`93.33% <100.00%> (ø)`
packages/auth-plugins/src/Login.tsx	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

mattrunyon · 2023-04-10T21:47:24Z

-        <Route exact path="/" component={AppInit} />
        <Route path="/notebook/:notebookPath+" component={AppInit} />
-        <Redirect from="*" to="/" />
+        <Route path="*" component={AppInit} />


Was the redirect causing an issue with the relative paths? It was to not show random URL paths that don't mean anything like /ide/somethingIMadeUp

Yea this doesn't work correctly with the './' at all, because it is just using the current URL path as the "basename".

I think instead, we should be getting the path from the import.meta.url (where are assets are). Not sure if the newer version of react-router-dom helps with this, but it has many breaking changes: https://reactrouter.com/en/main/upgrading/v5

For now I'll just fix up the base URL here.
Also it doesn't look like the /notebook/ redirect works anywhere right now.

Also, thinking about this even more - the BASE_URL set to ./ would screw up any routing we'd have with paths, since the index.html would be incorrect. e.g if I go to /ide/notebook/foo.py, it'll serve up the index.html, but the path to the assets will be incorrect, e.g.:

<script type="module" crossorigin src="./assets/index-05abad89.js"></script>

... Which would try and load /ide/notebook/assets/index-05abad89.js, which is of course incorrect.
Hmmm.

Opted for using BASE_URL=./, with the knowledge that deep-linking will fail unless the server/proxy is configured to rewrite the base tag on index.html. I think that's an acceptable compromise.
HashRouter sucks, it always writes a hash to the URL, and building relative URLs on the hash doesn't work right (e.g. you can't just do new URL('./mypath', document.baseURI) since that just gets rid of the full hash path).

devinrsmith · 2023-04-21T21:54:10Z

We discussed having the server parameterize the head base url - can you post a snippet of that section of the index.html @mofojed. From a purity standpoint, it's a bit hacky as now we aren't being a "transparent proxy".

There was also discussion around using query parameters instead of paths, and that could solve the issue.

Or, potentially a service worker.

mofojed · 2023-04-24T12:34:50Z

There's a note in the Create React App docs about this: https://create-react-app.dev/docs/deployment/#serving-the-same-build-from-different-paths

If you are not using the HTML5 pushState history API or not using client-side routing at all, it is unnecessary to specify the URL from which your app will be served.

We use Vite, but the same principles apply. We are serving up a static application, and client-side routing will not work correctly in this scenario. Either we don't do client-side routing (just using query params to specify paths), or we need server to rewrite the correct base. Of the two, just using query params for routing is certainly easier given our current architecture.

@devinrsmith The snippet that would need to be written in the index.html is similar to what we write in Enterprise (though it's written at Web UI Client build time, not dynamically by the server):

    <base href="/iriside/" />

devinrsmith · 2023-04-24T14:04:51Z

Ok - I'm still interested in the query param route so the proxy / server can be completely transparent about it.

If we need to test out the server/proxy re-writing <base href=...>, that's something I'll need to spend cycles on later...

mattrunyon · 2023-04-24T14:19:41Z

We could still do client side routing with the HashRouter, the URLs are just a little bit ugly since they become /foo/ide#iframe/table. I think the other downsides are you don't get proper forward/back support in the browser, but we don't really use that anyway

HashRouter lets the bundle stay at a single URL. Everything after the hash is client-side only for displaying routes from the single bundle.

mofojed · 2023-04-25T18:24:06Z

@dsmmcken how about using HashRouter for client side routing?

dsmmcken · 2023-04-25T19:21:04Z

No. Client side routing is a feature of the app.

mattrunyon · 2023-04-25T19:24:34Z

What client routing that we currently use would hash router not support? We don't use the history API in enterprise, so even though there are different URLs, you can't navigate forward/back. So hash router would work identically aside from the URL containing a #

mofojed · 2023-04-25T21:52:29Z

This cleans up some of the env vars and adds a base tag to the index.html.
It does not complete relative links entirely, since proxy will need to rewrite the base tag, but does not put us in a worse position. Still exploring proxy rewriting the base tag as an option.

- Merge in the latest main

- It was ugly. If server wants deep-linking, they need to handle it - Add notebook routes to the vite proxy

- Use URL path resolution instead of `path.resolve` - When using base `./`, and then relative jsapi path `../jsapi`, path.resolve wasn't working because it was trying to navigate to a local directory. Instead use URL path resolution, so the path doesn't go up past the root accidentally

- We want the preview to run with the base URL of /ide/ so tests go to the correct route - Fix proxy URLs to be anchored at the start

- It was previously still showing a spinner

mattrunyon · 2023-05-03T18:52:52Z

          const layoutStorage = new GrpcLayoutStorage(
            storageService,
-            import.meta.env.VITE_LAYOUTS_URL ?? ''
+            import.meta.env.VITE_STORAGE_PATH_LAYOUTS ?? ''


Layout and notebooks path might not need to be env variables (and might not even need to be part of the constructor), but that's a discussion/change for another PR

It was handy when we hadn't solidified the path yet. Would still need it somewhere global.

It's only used here. So we have an env variable to define a constant value used in 1 place specific to the app. Potentially useful if we have cases where we need to change it or proxy it. But right now we do neither

Fine to leave for now, just seemed a roundabout way to define what is now a constant value used in 1 location

mattrunyon · 2023-05-03T19:37:41Z

+# Any routes we define here
+VITE_ROUTE_NOTEBOOKS=notebooks/


Not a big fan of this being an env variable. Router we will almost certainly want static unless someone really wants to change the URL paths for some reason

Still need to have it globally somewhere. Where would you suggest?

I guess this is fine. Just feels odd to have notebooks/ and a /notebooks in here for slightly different uses

This is only used in the router and in ConsolePlugin, so I'm curious if there's a way to use it more cleanly in both. Not worth trying to figure out now though.

I think it's only used for linking from a markdown notebook right now. Might not need to have the URL if we know the file will just be relative to the GrpcFileService base

Yea the reason I had two is because the GrpcFileService does not necessarily have the same path as our route. They are different things, after all.

mofojed force-pushed the 1070-relative-links-2 branch from 91e3c14 to e1a2fa7 Compare April 4, 2023 22:07

mofojed changed the title ~~feat: 1070 relative links~~ feat: Relative links Apr 6, 2023

mofojed requested a review from mattrunyon April 6, 2023 13:10

mofojed self-assigned this Apr 6, 2023

mofojed marked this pull request as ready for review April 6, 2023 14:14

mattrunyon requested changes Apr 10, 2023

View reviewed changes

mofojed commented Apr 11, 2023

View reviewed changes

mofojed requested a review from mattrunyon April 25, 2023 21:51

mattrunyon requested changes Apr 28, 2023

View reviewed changes

Relative linking

2189ddf

- Merge in the latest main

mofojed force-pushed the 1070-relative-links-2 branch from 39928e3 to 2189ddf Compare May 3, 2023 00:08

mofojed added 5 commits May 2, 2023 20:42

Update base URI for notebooks...

cf16be8

Get rid of the hash router

c2ffb8d

- It was ugly. If server wants deep-linking, they need to handle it - Add notebook routes to the vite proxy

Fix up some base stuff

8e90533

- We want the preview to run with the base URL of /ide/ so tests go to the correct route - Fix proxy URLs to be anchored at the start

Fix path for logo on login screen

20f2a47

mofojed requested a review from mattrunyon May 3, 2023 16:44

mofojed added 2 commits May 3, 2023 13:41

Handle error with authentication properly

7dd6ac3

- It was previously still showing a spinner

Only show an error message when it's set

c367e2e

mattrunyon requested changes May 3, 2023

View reviewed changes

Clean up after review

d47595f

devinrsmith mentioned this pull request May 3, 2023

Redirect from / to /ide/ should not append the server's own IP address deephaven/deephaven-core#3786

Closed

mattrunyon approved these changes May 3, 2023

View reviewed changes

mofojed merged commit f440eb9 into deephaven:main May 3, 2023

mofojed deleted the 1070-relative-links-2 branch May 3, 2023 21:10

github-actions Bot locked and limited conversation to collaborators May 3, 2023

Conversation

mofojed commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devinrsmith commented Apr 21, 2023

Uh oh!

mofojed commented Apr 24, 2023

Uh oh!

devinrsmith commented Apr 24, 2023

Uh oh!

mattrunyon commented Apr 24, 2023

Uh oh!

mofojed commented Apr 25, 2023

Uh oh!

dsmmcken commented Apr 25, 2023

Uh oh!

mattrunyon commented Apr 25, 2023

Uh oh!

mofojed commented Apr 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mofojed commented Apr 4, 2023 •

edited

Loading

codecov Bot commented Apr 4, 2023 •

edited

Loading