refactor: Change embed-grid and embed-chart to redirects #1873
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1873 +/- ##
==========================================
+ Coverage 46.11% 46.23% +0.12%
==========================================
Files 636 645 +9
Lines 38070 38241 +171
Branches 9627 9672 +45
==========================================
+ Hits 17556 17682 +126
- Misses 20461 20506 +45
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| const url = new URL(window.location); | ||
| url.pathname = '/iframe/widget/'; |
There was a problem hiding this comment.
Should be using baseURI, not window.location:
| const url = new URL(window.location); | |
| url.pathname = '/iframe/widget/'; | |
| const url = new URL('/iframe/widget/', document.baseURI); |
There was a problem hiding this comment.
Ya I think I need to adjust it to a relative path and add a base tag then. In the existing version, we have base=./ so baseURI is actually /iframe/chart or /iframe/table
What this doesn't allow (and neither does our existing base) is specifying a base that isn't ./ or rewriting it with a web server. If our base is ever rewritten, the jsapi and iframes will try to load from paths that likely don't exist
There was a problem hiding this comment.
Also, if I combine instead of just changing url.pathname after, then the URL params get dropped
There was a problem hiding this comment.
Yea true it assumes we're at a path like iframe/table already:
BASE_URL=./
# We assume embed-grid is served at nested path, e.g. '/iframe/grid'
VITE_CORE_API_URL=../../jsapi
VITE_MODULE_PLUGINS_URL=../../js-plugins
Devin has rewriting the base before to have two Deephaven instances running on the same host; but that's beside the point.
Also good point about dropping params.
|
Changed this to just use vite to build embed-grid and embed-chart still. All it does is copy the index into the build folder. This way there's no issues of people accidentally committing stale build artifacts since the folder was being un-ignored with the first change. Will publish an alpha and test everything is working properly |
| <!doctype html> | ||
| <html lang="en"> | ||
| <head> | ||
| <base href="./" /> |
There was a problem hiding this comment.
I was just trying to make this simple (using Vite default config). I don't see any reason BASE_URL needs to be easily changeable at build time as we don't support anything other than ./
packages/embed-grid/index.html
Outdated
|
|
||
| <script> | ||
| // Can't be an HTML meta redirect because we need to keep the search params | ||
| const url = new URL('../../iframe/widget/', document.baseURI); |
There was a problem hiding this comment.
May as well just do ../widget ...
| const url = new URL('../../iframe/widget/', document.baseURI); | |
| const url = new URL('../widget/', document.baseURI); |
We could have env vars that you set when building to reference where the new iframe path should be, but this is fine... if you change the path, you'll need to rewrite the path in this index.html file.
There was a problem hiding this comment.
Ya I figured these were simple enough redirects that adding an env file just adds an unnecessary layer of pass through. Not any gain IMO when we're replacing a single use value with a single use variable instead and we're ultimately trying to remove these separate embed packages
|
Just tested again w/ the latest commit alpha and |
| <!doctype html> | ||
| <html lang="en"> | ||
| <head> | ||
| <base href="./" /> |
Publishing an alpha with the
embed-refactorlabel so we can test with a local DHC buildThis changes embed-grid and embed-chart to just be simple redirects to
/iframe/widget. This should reduce our footprint in the install size as this is about half of the size we contribute currently.BREAKING CHANGE:
@deephaven/embed-griddoes not handle messages to the iframe for filtering or sorting the grid any more