-
Notifications
You must be signed in to change notification settings - Fork 760
fix: marimo development preview #5403
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| */ | ||
| export const connectionAtom = atom<ConnectionStatus>({ | ||
| state: WebSocketState.CONNECTING, | ||
| state: isStaticNotebook() ? WebSocketState.OPEN : WebSocketState.CONNECTING, |
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 feels suspicious because it only appears to be needed when serving HTML with local assets; might not be the right fix.
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.
Will test locally, but looks fine
14c518d to
1a89a76
Compare
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 looks great! Wondering if instead of checking the type of notebook globally in the connection atom, we can try to "fix" the static case at the websocket implementation level?
// In frontend/src/core/websocket/StaticWebsocket.ts
addEventListener(type: string, listener: EventListener): void {
if (type === "open") {
this.onopen = listener;
// Trigger open event immediately since static websockets are always "open"
queueMicrotask(() => this.onopen?.(new Event("open")));
}
}This way the StaticWebsocket properly simulates a WebSocket that immediately opens, and we avoid leaking static mode awareness into the global connection state.
|
Yea that's much better. I'll make the change before merging. Thank you! |
|
Hmm... just tried my change locally and still see the spinner. I can poke around a bit, but guessing the |
|
@manzt let me know if you want changes made, or if okay to merge as is |
* actually generate the notebook session and outputs * for static notebooks, initialize the "websocket" as open
for more information, see https://pre-commit.ci
b5a4619 to
0092e85
Compare
|
Still getting used to procedure with taking over other's PRs... I was able to check this out locally and get the following to work with: wget https://gist.githubusercontent.com/manzt/f2a9299d913dc9801c639134db6e06e8/raw/f9ca2d565a3df3597a0eb85fbec230eac71b17f7/mo-3226.py
make fe
uv run --with matplotlib,numpy marimo development preview mo-3226.pydiff --git i/frontend/src/core/network/connection.ts w/frontend/src/core/network/connection.ts
index daa8d339..67f58b5b 100644
--- i/frontend/src/core/network/connection.ts
+++ w/frontend/src/core/network/connection.ts
@@ -1,7 +1,6 @@
/* Copyright 2024 Marimo. All rights reserved. */
import { atom } from "jotai";
import { waitFor } from "../state/jotai";
-import { isStaticNotebook } from "../static/static-state";
import { type ConnectionStatus, WebSocketState } from "../websocket/types";
/**
@@ -9,7 +8,7 @@ import { type ConnectionStatus, WebSocketState } from "../websocket/types";
* Initialized to CONNECTING for normal mode, OPEN for static mode.
*/
export const connectionAtom = atom<ConnectionStatus>({
- state: isStaticNotebook() ? WebSocketState.OPEN : WebSocketState.CONNECTING,
+ state: WebSocketState.CONNECTING,
});
export function waitForConnectionOpen() {
diff --git i/frontend/src/core/websocket/StaticWebsocket.ts w/frontend/src/core/websocket/StaticWebsocket.ts
index d4120366..981a23e2 100644
--- i/frontend/src/core/websocket/StaticWebsocket.ts
+++ w/frontend/src/core/websocket/StaticWebsocket.ts
@@ -17,9 +17,20 @@ export class StaticWebsocket implements IReconnectingWebSocket {
onmessage = null;
onopen = null;
- addEventListener(type: unknown, callback: unknown, options?: unknown): void {
- // Noop
+ addEventListener(
+ type: string,
+ callback: EventListener,
+ _options?: unknown,
+ ): void {
+ // Normally this would be a no-op in a mock, but we simulate a synthetic "open" event
+ // to mimic the WebSocket transitioning from CONNECTING to OPEN state.
+ if (type === "open") {
+ queueMicrotask(() => {
+ callback(new Event("open"));
+ });
+ }
}
+
removeEventListener(
type: unknown,
callback: unknown,I'm happy to apply the diff directly if that is preferred! |
don't leak static notebook awareness to connection object
|
@manzt thanks! just applied it |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.14.8-dev6 |
## 📝 Summary quarto `0.14.7` is broken. I'm having trouble confirming this is the issue by replicating it locally, since I can't replicate the breakage. My dev served assets seem to work in general. But `being islands` != `being wasm` as is, and I think some of the logic around sockets gets lost (0.14.6 works, so this may be the offending PR: #5403) @akshayka --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This PR fixes `marimo development preview` for previewing static HTML
using local assets. Previously the generated HTML didn't include outputs
(ie the notebook wasn't run), and also got stuck on attempting to
connect to the frontend ("Connecting..." spinner)
* actually generate the notebook session and outputs
* for static notebooks, initialize the connection as open
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: dylan <dylan@marimo.io>
## 📝 Summary quarto `0.14.7` is broken. I'm having trouble confirming this is the issue by replicating it locally, since I can't replicate the breakage. My dev served assets seem to work in general. But `being islands` != `being wasm` as is, and I think some of the logic around sockets gets lost (0.14.6 works, so this may be the offending PR: marimo-team#5403) @akshayka --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This PR fixes `marimo development preview` for previewing static HTML
using local assets. Previously the generated HTML didn't include outputs
(ie the notebook wasn't run), and also got stuck on attempting to
connect to the frontend ("Connecting..." spinner)
* actually generate the notebook session and outputs
* for static notebooks, initialize the connection as open
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: dylan <dylan@marimo.io>
## 📝 Summary quarto `0.14.7` is broken. I'm having trouble confirming this is the issue by replicating it locally, since I can't replicate the breakage. My dev served assets seem to work in general. But `being islands` != `being wasm` as is, and I think some of the logic around sockets gets lost (0.14.6 works, so this may be the offending PR: marimo-team#5403) @akshayka --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This PR fixes `marimo development preview` for previewing static HTML
using local assets. Previously the generated HTML didn't include outputs
(ie the notebook wasn't run), and also got stuck on attempting to
connect to the frontend ("Connecting..." spinner)
* actually generate the notebook session and outputs
* for static notebooks, initialize the connection as open
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: dylan <dylan@marimo.io>
## 📝 Summary quarto `0.14.7` is broken. I'm having trouble confirming this is the issue by replicating it locally, since I can't replicate the breakage. My dev served assets seem to work in general. But `being islands` != `being wasm` as is, and I think some of the logic around sockets gets lost (0.14.6 works, so this may be the offending PR: marimo-team#5403) @akshayka --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This PR fixes
marimo development previewfor previewing static HTML using local assets. Previously the generated HTML didn't include outputs (ie the notebook wasn't run), and also got stuck on attempting to connect to the frontend ("Connecting..." spinner)