Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Add connection resume, client state tracking, robust reconnect logic, reconnecting hooks, more #1204

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
+1,313 −229
Draft

[WIP] Add connection resume, client state tracking, robust reconnect logic, reconnecting hooks, more #1204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8f6c8ec
Fix static type hints for component decorator (#2)
JamesHutchison Mar 5, 2024
efa547f
Connection resume (#1)
JamesHutchison Mar 5, 2024
a7374d0
Follow up work (#3)
JamesHutchison Mar 5, 2024
ef8537f
add location to serialization (#4)
JamesHutchison Mar 5, 2024
e23e43d
Fix client not reloading on hash mismatch (#5)
JamesHutchison Mar 6, 2024
d36a982
Fix user mouse / scroll activity not actually triggering a reconnecti…
JamesHutchison Mar 6, 2024
6118197
fix state recovery error not propagating (#7)
JamesHutchison Mar 8, 2024
09eb44c
Move socket loop interval to a variable (#8)
JamesHutchison Mar 8, 2024
5f5562f
mypy typing updates (#9)
JamesHutchison Mar 12, 2024
6203da5
add overload (#10)
JamesHutchison Mar 12, 2024
9704cbc
finally fix priority typing (#11)
JamesHutchison Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/js/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 9 additions & 17 deletions src/js/packages/@reactpy/client/src/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function Layout(props: { client: ReactPyClient }): JSX.Element {

useEffect(
() =>
props.client.onMessage("layout-update", ({ path, model }) => {
props.client.onLayoutUpdate((path: string, model: any) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue A: bug fix for when server would return "layout-update" before the client was ready.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this issue only exist due to the client state management stuff in this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a long while. My intuition is "no" but one would need to check the code. IIRC there's a delay from connecting to setting up the handlers and no way to set up the handlers ahead of time, but this is top of my head and I don't have the code in front of me.

if (path === "") {
Object.assign(currentModel, model);
} else {
Expand Down Expand Up @@ -125,23 +125,15 @@ function ScriptElement({ model }: { model: ReactPyVdom }) {
(value): value is string => typeof value == "string",
)[0];

let scriptElement: HTMLScriptElement;
if (model.attributes) {
scriptElement = document.createElement("script");
for (const [k, v] of Object.entries(model.attributes)) {
scriptElement.setAttribute(k, v);
}
if (scriptContent) {
scriptElement.appendChild(document.createTextNode(scriptContent));
}
ref.current.appendChild(scriptElement);
} else if (scriptContent) {
const scriptResult = eval(scriptContent);
if (typeof scriptResult == "function") {
return scriptResult();
}
const scriptElement: HTMLScriptElement = document.createElement("script");
for (const [k, v] of Object.entries(model.attributes || {})) {
scriptElement.setAttribute(k, v);
}
if (scriptContent) {
scriptElement.appendChild(document.createTextNode(scriptContent));
}
}, [model.key, ref.current]);
ref.current.appendChild(scriptElement);
}, [model.key]);
Comment on lines +128 to +136
Copy link
Author

@JamesHutchison JamesHutchison Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue B: bug fix for inconsistent script rendering that resulted in double execution of scripts. Scripts also always are returned to the DOM for user inspection rather than executed directly.

The current behavior was to immediately execute a script with no properties. If it had properties, it went to the DOM instead. These different code paths likely hid the bug where scripts were double executing.

The fix for the double execution was to remove ref.current from the dependency list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm testing out this implementation in #1239 but it seems that it breaks script re-execution when the script key (or content) changes.

Copy link
Author

@JamesHutchison JamesHutchison Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like the issue I've seen with inputs. You'll change the key and value on the input but existing value sticks around. It's an issue if you have a list of objects with inputs, update the list, then redraw. The browser shows the old value and doesn't update it.

The workaround IIRC is to wrap it and change the key on what its wrapped in.

I was under the impression my code change to support reconnection was the cause but maybe its not? If that's the case, it sounds like its a bug that affects everything, not just scripts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just to call this out, I'm not sure having script tags execute multiple times because they changed is necessarily a good practice. I can think of a couple scenarios where implementation changes such as how renders are sent to the client might create behavioral changes due to "in-between" states getting skipped. Seems like it should always be one script one execution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could agree with that - Given that sequential renders that affect the same components are "optimized out" by layout.py, there is definitely a reality where things could get skipped.

I am also aware that layout.py has some bugs, so it's on my plate to rewrite that.

See issue:


return <div ref={ref} />;
}
Expand Down
9 changes: 7 additions & 2 deletions src/js/packages/@reactpy/client/src/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ export type LayoutEventMessage = {
data: any;
};

export type IncomingMessage = LayoutUpdateMessage;
export type OutgoingMessage = LayoutEventMessage;
export type ReconnectingCheckMessage = {
type: "reconnecting-check";
value: string;
}

export type IncomingMessage = LayoutUpdateMessage | ReconnectingCheckMessage;
export type OutgoingMessage = LayoutEventMessage | ReconnectingCheckMessage;
Comment on lines +15 to +21
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue C: Message updates. I think looking at it out now I didn't finish this with the most recent messages, such as "state-update"

export type Message = IncomingMessage | OutgoingMessage;
Loading