-
Notifications
You must be signed in to change notification settings - Fork 320
feat(dev-hub) Pyth Pro Playground #3346
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
apps/developer-hub/src/components/Pages/PlaygroundPage/index.tsx
Outdated
Show resolved
Hide resolved
| variant="primary" | ||
| size="lg" | ||
| onPress={handleRunCode} | ||
| className={styles.runButton ?? ""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is rather annoying. we should allow for a nullish className to be set on our components (not something you need to fix, just thinking out loud 😅 )
apps/developer-hub/src/components/Playground/PropertiesSelector/index.tsx
Show resolved
Hide resolved
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | ||
| let isClosed = false; | ||
|
|
||
| const stream = new ReadableStream({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 we may need to do a deeper dive on the usage of the EventStream API here. it's a good usage here in Next (since we cannot use server-side websockets in Next), but there is an opportunity to move this API out of developer hub and into the shared-lib, so we can standardize the API and also provide some React hooks that work easily with it, without needing to build something custom per-app (thinking out loud here, as this is 100% something that we will need in other places)
| }; | ||
| websocket = new WebSocket(wsUrl, wsOptions); | ||
|
|
||
| websocket.addEventListener("open", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually not going to work once deployed. it works locally, because you have a single, persistent instance that is always up. However, once deployed to Vercel, the app cluster will be scaled up and down dynamically, so the server that opened this websocket instance will likely be killed, which will cause problems that manifest as Pyth API slowness and instability (definitely not a thing to promote 😓 ).
Let's revisit this. We may need to have a bespoke server, written in Rust or Javascript, depending on which Pyth Client we want to use, that only handles streaming for the DevHub docsite and the upcoming Admin portal


Summary
Rationale
How has this been tested?