-
-
Notifications
You must be signed in to change notification settings - Fork 762
Project BASE - Faster HTTP Reconnect #4719
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
Very interesting idea, @evnchn! Seems to be a comparatively small change with a big impact. 👌🏻 |
This comment was marked as resolved.
This comment was marked as resolved.
#4719 (comment) addressed with f955e42 |
So @rodja and I was discussing how this PR relates to Single-Page Application in another thread. As a recap, the definition of a Single-Page Application is:
For this PR it is itself mostly oriented towards solving point 1, since as you know the single-megabit uplink hate page loads. However, as we have seen in Lines 64 to 91 in fa34e46
Point 3 is real, though, and I should have thought of that as the "performance-focused guy" on the team 😅. Anyways, I think it is doable, if we:
Point 2 elaboration: Proposed elements JSON string:
|
I'm not sure if a combination of this PR with a UI element cache can provide a sufficient SPA implementation. From my point of view, a NiceGUI SPA is not only defined by "Load in only the updated content" but also "Not rerun backend code". And to generate a JSON string like |
I'd like to disagree by pointing out a half-baked (IMO) SPA, namely GitHub itself. Ever noticed, when the number of PRs has gone up, that some old tabs stays on the old PR count, until you reload or go inside and outside of NiceGUI repository? I sure did, as the source of those PRs 😅 This is because, GitHub does not re-execute the backend code responsible to calculating how many PRs are there and update the header, while just pulling in the body. But this could work for this PR: def get_number_of_PRs():
# some function
def common_header():
ui.label(f'Number of PRs: {get_number_of_PRs()}')
@ui.page('/page1')
def page1():
common_header()
# content
@ui.page('/page2')
def page2():
common_header()
# content Every single time you re-navigate using soft reload, it re-executes the In contrast, assuming #4821 syntax, this would keep the stale PR count on navigation: def get_number_of_PRs():
# some function
def common_header():
ui.label(f'Number of PRs: {get_number_of_PRs()}')
@ui.page('/')
@ui.page('/{_:path}') # tells FastAPI/Starlette that all path's should land on our SPA
def index():
common_header()
ui.sub_pages({'/': child, '/other': child2})
def child():
ui.link('goto other', '/other')
def child2():
ui.link('goto main', '/') Rather, to change the common header, we need to actively pass the elements which may change into the subpage, as prescribed in #4821 (comment) @ui.page('/')
@ui.page('/{_:path}')
def index():
title = ui.label()
ui.sub_pages({
'/': lambda: child(title),
'/other': lambda: child2(title)
})
def child(title: ui.label):
title.set_text('child title')
ui.link('goto other', '/other')
def child2(title: ui.label):
title.set_text('other title')
ui.link('goto main', '/') Overall, I am inclined to believe, if implemented well, this PR can be a better implementation to SPA than #4821. |
from nicegui import ui
def reused_ui_elements():
ui.label('This is a label that can be reused.')
ui.table(rows=[{'id': 1, 'name': 'Alice'}, {'id': 2, 'name': 'Bob'}])
ui.markdown('#### This is a markdown element that can be reused.')
ui.link('Go to main page', '/')
ui.link('Go to another page', '/another')
@ui.page('/')
def main_page():
ui.label('This is the main page.')
reused_ui_elements()
@ui.page('/another')
def another_page():
ui.label('This is another page.')
reused_ui_elements()
# Copy from main.py
ui.add_body_html('''<script>
// Your custom function
function customFunction(event) {
let target = event.target;
// Traverse up the DOM tree to find the nearest anchor tag
while (target && target.tagName !== 'A') {
target = target.parentElement;
}
if (target && target.tagName === 'A') {
console.log('Link clicked!'); // Replace with your custom logic
console.log(target.href); // Log the href of the clicked link
try {
window.softReload(target.href);
event.preventDefault();
} catch (e) {
console.error('Error in softReload:', e);
}
}
}
// Attach the event listener to the document
document.addEventListener('click', customFunction);
window.addEventListener("popstate", (event) => {
console.log("popstate", event.state);
softReload(window.location.href, event.state?.x, event.state?.y);
});
</script>
''', shared=True)
ui.run() Betwen page soft reloads, it detects that the elements in As such, Project REBASE (name loosely inspired from ![]() Note that the table is re-transmitted, because I did not fully steal the logic from #4796. If I did, then it would detect that the table, only the event handlers are changed, and pass something like the below (transmitting only the new event handlers):
Seems like this is a much easier approach over SPA, with NiceGUI taking up the heavy lifting of finding identical elements between pages, instead of user having to divide their content between the main page and |
The logic in 21c2fad is:
|
I'm really fond of the friendly competition here, @evnchn. It is awesome how you transformed this "Faster HTTP Reconnect" into a SPA concept 😃 For me it is not an either #4821 or this PR. They server both quite different purposes. Even if this PR might be a the groundwork for an "SPA behind-the-scenes", at the beginning it was so neat and tight. Pushing the concept towards SPA might be better off in a follow up pull request. I just tested the implementation and directly ran into errors where the page switch was broken: |
About your comment about full-page reevaluation vs partial execution:
While this can be nice in some circumstances, it can also be very problematic in others. For example if you have a costly database request in the outer layout. Or a search filter which should not be reset. In short: re-execution of the whole page might be costly. It depends on the scenario. I suggest to
|
…ication" This reverts commit 21c2fad.
First of all, this PR is evolving so quickly! I was cleaning my room, and it took me a solid 15 minutes to re-familarize myself with this PR. Now, we need a summary.
@rodja, I am so sorry for shipping broken commit in 21c2fad, leading to a general bad impression of Project BASE. I was too happy at looking at my key-diffing code serving small payloads, that the page was broken and I did not realize. Apparently, (1) there must have been some problem with the logic which diffs the elements on a key-by-key basis, in a sense that old keys are not dropped, and (2) even if I fixed it, some elements are still messed up, since Vue doesn't force a re-render, and so the old element sticks around. Check the below, where I made sure to manually compare the elements: ![]() JSON-wise, key 97 is fine. But, c97 is still messed up ![]() So, we really can't proceed with key-level diffing, unless we dig deeper into Vue, which we need to get #4828 done first, in order to be in a better headspace to solve these kinds of issue. But, regardless, assuming we do the commit before that, we still get project REBASE with SPA-like functionality. For your suggestions:
I would then need to achieve the "Fast reconnection in case client destroyed, because disconnected over the timeout duration / server reboot (STILL to be implemented 😅)" which I have dragged on.
Sure, but I think you can handle it, right?
But I think this part is what the PR truly shines, and I am not sure if it would be better to finalize this before we ship the feature. Otherwise, I feel like |
Moreover, as we can see after reverting 21c2fad, push/pop state + sending only the changed parts already works on documentation. So on that front, I'm not sure what work we have to do, other than pytest and documentation. |
Ah ok. Only reverting the key-diffing but keeping pushState/popState might also be a good cut. But still I can break the documentation page quite easily. For example if you open |
Can you send me a reproduction? Though, I'm thinking if we can't get it to work after enough attempts, that we remove the duplication removal, and just send the entire unmodified element dictionary over... This is because, we have #4796 on our side, if we want to do deduplication as well. Moreover, the bugs we are seeing looks very similar to #4828, so it looks like it'd not be an easy problem to solve. |
Although I fixed @rodja's issue on repeated clicks, there is a big problem with this PR: memory leak in the browser-side ![]() I have banged my head against the wall for an hour to no avail. If we do not address this, this PR cannot proceed. I have tried:
All attempts came up dry and I am lost as to how I can fix it, especially since the JavaScript-side is so complex... At least I am pretty sure that #4821 won't suffer from this issue, since it doing things in the NiceGUI-layer. |
This PR introduces a HTTP-based special mechanism for loading in the page which is must faster than the browser.
Brief explainer
The server, upon the setting of a special private header
'X-NiceGUI-Client-ID'
, instead of serving HTML, proceeds instead to serve it in JSON machine-readable format.Which, the special JavaScript function
softReload
will:createApp(...)
vue_script
for the Plotly and similar elements which rely on it#app
Essentially, it does what a page load will do, but without a page load.
Name meaning of BASE
BASE stands for "Because Asked Server Explicitly", which is an (admittedly not very good) acryonym reflecting on the nature that the client asks the server explicitly for reconnection advice via HTTP, including the crucial Client ID.
This contrasts with project ACID #4552 before it, which the client can say it had any arbitrary Client ID via Socket.IO, and the server must bend around it by then suddenly spawning in the Client, bypassing HTTP layer entirely.
Advantages
Advantages shared common between ACID and BASE:
Advantages exclusive to BASE:
'X-NiceGUI-Client-ID'
is not set in headerRemaining tasks
softReload
doesn't set browser URL barsoftReload
code to be unified? This can further ensure exact same behaviour.Testing code
My testing code
Results:
vue_scripts
works.