Refactor demo to refresh token automatically #6
Conversation
| const { widgetUrl, token } = await getScopedToken(); | ||
| const originalWidgetUrl = new URL(widgetUrl); | ||
| const newWidgetUrl = new URL( | ||
| `${import.meta.env.VITE_AB_WEBAPP_URL}${originalWidgetUrl.pathname.toString()}?${originalWidgetUrl.searchParams.toString()}` |
There was a problem hiding this comment.
Need to update this to conditionally use VITE_AB_WEBAPP_URL only if it's set.
| </div> | ||
| <script type="module"> | ||
| import { EmbeddedWidget } from "@airbyte-embedded/airbyte-embedded-widget"; | ||
| import { EmbeddedWidget } from "../src/EmbeddedWidget.ts"; |
There was a problem hiding this comment.
I don't have a strong opinion on how we import this... Evan had suggested changing this from directly importing the file to importing it using @airbyte-embedded originally... I am guessing so it's more production-like.
There was a problem hiding this comment.
Yeah I guess this is the tension between "the demo is for dev work" and "the demo is a demo". I find it annoying to have to remember to go change the import path every time I want to work on the EmbeddedWidget.
What if we just move the current demo app into a dev directory, and we keep a more polished demo in the demo directory? That would also allow us to keep all our dev niceties (client side token refreshing, import path, overrides) separate from the actual "demo" that operators might spin up.
There was a problem hiding this comment.
I have a pr introducing a version with a server that solves the token fetching also. Let's do that instead?
There was a problem hiding this comment.
sounds good, I split out the .updateToken() method into this PR since your PR does not seem to do that yet: #13
|
With removing the demo.js, the code in the HTML file becomes a lot more complex. We want that file to be easily digestible for an Operator running a POC of embedded. They won't care about local overrides. IMO the local overrides option should remain separate. |
joeykmh
changed the title
|
closing in favor of #11 |
This PR does two things:
EmbeddedWidgetto add anupdateToken()methodThe way the demo does the token refreshing is unsafe for a production application, because we are exposing the
client_idandclient_secretin the browser. However, I think this is way faster and easier to get up and running with. Users just have to paste in some static values into thedemo/.envfile and they can use the demo without the session expiring.We would probably want a short guide somewhere about how to do this more safely in a production environment (e.g. use your own backend to fetch a scoped token for the given user, pass that to your webapp, then call
EmbeddedWidget.updateToken(newToken)whenever appropriate. Putting a production-grade implementation in the demo feels excessive, though.If this approach looks good, I think we can get rid of
demo/scripts/dev.jsand update the readme to instruct users to use theirclient_idandclient_secretdirectly.