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.

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

Introduce API to build Single Page Applications (SPAs) #2811

Draft
wants to merge 139 commits into
base: main
Choose a base branch
from

Conversation

Alyxion
Copy link
Contributor

@Alyxion Alyxion commented Apr 3, 2024

NiceGUI is for us at Lechler a really awesome solution and step forward from Streamlit in regards of the visualization of live and streaming data as it puts the dev far more in control of which sub elements and page regions are updated when.

On the other hand it is still lacking three for us very crucial features Streamlit offers:

  • A persistent WebSocket connection for the whole user session (per tab)
  • A data storage possibility per tab to enable the user to create multiple app instances.
  • In-memory storage of complex objects (e.g. pandas data tables)

image

This (still work in progress) pull request tries to resolve at least most of the points above. It shall not yet resolve the situation that a user has an unstable internet connection and thus looses the connection to a server completely and needs to reconnect.

Persistent connection

In a scenario where you want to serve your NiceGUI solution not to hundreds of users there is after a certain point no way around scaling the solution over multiple processes, CPUs or over multiple servers.

If you need to load/keep alive large amounts of data per user w/o involving an external database this requires that the whole user session, even between page changes, is bound to one single process on one specific server. Streamlits uses a SPA approach here thus it creates a WebSockets connection once and all follow-up page and tab changes are just virtual thus changing the URL and browser history in the browser using pushstate but never really loading a new page using GET.

As discussed in the Add app.storage.tab or similar (1308) and in Discord there are several use cases where this is crucial to retain in-memory data on a "per session" basis, see below, which consequently requires that there is such a session in the first place.

Per tab storage

A data storage possibility per tab is cruicial to enable the user to create multiple app instances with different login credentials, configurations and views on a per tab basis. This is on purpose volatile so that user-credentials, critical business data etc. are gone once the browser tab was closed and the connection timed out. This shall match the current behavior of st.session_state.

In-memory storage of complex objects

The possibility to store living, non-JSON compatible objects such as Pandas tables, ML model weights etc. on a "per tab" basis and make them as easy accessible among different pages, global helper classes etc. as currently app.storage.user.

Update: Extracted the app.storage.session feature into a separate pull request 2820

Open ToDos/Topics we need to adress

  • simplify API by allowing Outlet without yield (and then get rid of OutletView and rename Outlet to Content or similar); see Simplify API by merging Outlet and View Alyxion/nicegui#4 for WIP
  • Nested outlets can't be async yet.
  • put all routing into a single place: not some in Outlet and similar stuff in SinglePageTarget; and do we have to do separate routing in frame.js?
  • rethink architecture of "builder routing" as opposite to "real routing" via backend (ui.navigate) or frontend (ui.link)
    • maybe get rid on on_navigate in favour of a class registration in the style of Starlet middlewares?
    • There are situations when you can already see in the Outlet than building the view can't succeed, e.g. you already detect the db is down, user is not authed etc. etc. In such siuations it might definitely make sense to be able to block all views / dont try to build them. Either via an Exception or e.g. returning rather than yielding.
  • simplify or get rid of SinglePageTarget.valid member variable
  • Quite regularly we see the "wait for the client.connected" timeout. Why? And how to do better?
  • find a better name for user_data (it's more analog to parameters in function calls)
  • buildier_utils and SinglePageTarget have similar if/else instance checks; we should consolidate
  • write documentation
  • cleanup examples (remove old SPA, improve/simplify the new "complex" one)

@Alyxion Alyxion changed the title Support for per browser-tab session data and a persistent per-session between page changes Support for per browser-tab session data and a persistent per-session connection between page changes Apr 3, 2024
Michael Ikemann and others added 11 commits April 3, 2024 14:36
Integrated single page app into Client.open so navigation to SPA pages is redirected.
Fixed bug with forward and backwards navigation between SPA pages.
Collecting data from original pages to able to apply the original page title.
Integrated single page app into Client.open so navigation to SPA pages is redirected.
Fixed bug with forward and backwards navigation between SPA pages.
Collecting data from original pages to able to apply the original page title.
Fixed a bug which could occur when open was called before the UI was set up
…s registry in Client.

General clean-up
Added titles to sample app
Added docu to SPA
@Alyxion Alyxion force-pushed the feature/client_data branch from 4b89fb3 to 13f29ac Compare April 3, 2024 12:40
@falkoschindler falkoschindler requested a review from rodja April 3, 2024 14:06
…the structure of the root page, the possibility to override the class and implement custom routing and to react to the creation of sessions.

* Added samples for the single page router
* Refactored the Login sample with the new possibilities and making use of Pydantic as an example for a cleaner code base
@rodja
Copy link
Member

rodja commented Apr 4, 2024

Thank you @Alyxion. There are a lot of interesting things in here. While you are right that a certain kind of applications my want to combine SPA and "per tab storage", it would make review and discussions simpler if you could create two separate pull requests for these features.

…in the current Client instance - which in practice means "per browser tab".
@Alyxion
Copy link
Contributor Author

Alyxion commented Apr 4, 2024

Hello @rodja, sure. As this Pull Request here is anyway just a draft yet and the amount of changes of the per-tab data was manageable I extracted the lines into the separate request #2820 and added a unit test to it. As this one is dependent on the other I will keep it the way it is for now.

@Alyxion
Copy link
Contributor Author

Alyxion commented Apr 5, 2024

Added support for query and URL path parameters such as required by the modularization example.

https://github.com/Alyxion/nicegui/blob/feature/client_data/examples/modularization/main.py works really like a charm now in regards of user experience when switching pages.

At the moment it still throws a "Found top level layout element "Header" inside element "SinglePageRouterFrame". Top level layout elements should not be nested but must be direct children of the page content. This will be raising an exception in NiceGUI 1.5" warning due to the fact that it does not know yet that it actually is in the page content in that case.

Update: Fixed the warning. PageLayout now also accepts the SPA root as valid parent for top level elements.

@rodja
Copy link
Member

rodja commented Feb 3, 2025

And some more question/remarks:

  1. Do we need the SinglePageTarget.valid property? I think Outlet.resolve_target should simply return None if it does not matches the routes.
  2. Can we get rid of the route matching in SinglePageTarget and completely do it in the `Outlet? It would better suit their responsibilities.
  3. Do we really need await ui.context.client.connected(30.0) # to ensure storage.tab and storage.client availability in Outlet.setup_page?
  4. Can you list all the features you implemented? Or even better: write up the tests to ensure the features are working? In dc6185d I started with the obvious ones. But there is so much more. Path matching, user_data, setting titles, ... while I was looking through the code I could barely change anything without fear of breaking it.

@Alyxion
Copy link
Contributor Author

Alyxion commented Feb 3, 2025

Hello Rodja,

thank you for the the detailed review. I think many of the points you mentioned should be resolvable quite quickly, others will take some time. Will get back to your points next week.

@whoamiafterall
Copy link

whoamiafterall commented Feb 4, 2025

Hey, may be a little off-topic here, but regarding point 8 it might be helpful.
We're currently using this example in our code:
https://github.com/zauberzeug/nicegui/tree/main/examples/single_page_app

And we run into navigation issues (buttons not working and so on) all the time if we don't put await ui.context.client.connected() in the first line of every "view" which i guess refers to "outlet.page" in this PR's terminology.
We still do run into issues sometimes even with it added though and can't really figure out why.

If you think it helps i can provide the corresponding error logs (Timeout errors from router mainly) and code snippets.
Or do you think i should open an issue on the current SPA_example (Haven't done that because i'm waiting for this to be merged)?

@Alyxion
Copy link
Contributor Author

Alyxion commented Feb 10, 2025

Hey, may be a little off-topic here, but regarding point 8 it might be helpful. We're currently using this example in our code: https://github.com/zauberzeug/nicegui/tree/main/examples/single_page_app

And we run into navigation issues (buttons not working and so on) all the time if we don't put await ui.context.client.connected() in the first line of every "view" which i guess refers to "outlet.page" in this PR's terminology. We still do run into issues sometimes even with it added though and can't really figure out why.

If you think it helps i can provide the corresponding error logs (Timeout errors from router mainly) and code snippets. Or do you think i should open an issue on the current SPA_example (Haven't done that because i'm waiting for this to be merged)?

Thank you @whoamiafterall . Actually the original SPA sample when I started this PR (meanwhile nearly a year ago) had some issues with the back button, though 4 months ago something has been patched. But its diverged soooo far to this PR now.

Regarding the ui.context.client.connected(): My first guess is that if you wait for being connected, you can also be sure that the JavaScripts where loaded and thus the window.addEventListener("popstate", (event) is in place.

This PR actually even enforces the ui.context.client.connected() under the hood so you at least would not have to add it to every method again.

@Alyxion
Copy link
Contributor Author

Alyxion commented Feb 10, 2025

First of all sorry for the delay, but at the moment I'm quite busy. Thanks for your detailed review once again.

Hi @Alyxion, I found a bit time to further look into the pull request. While by no means complete or well structured, I have some questions and remarks:

  1. The test_routing_url is failing. I could not quickly figure out why the url path is not updated anymore. Maybe you have an idea?

I just verified the code again, it should be correct. Before a navigation execution is actually executed first the callback is checked, than the "config" (ex "SinglePageRouterConfig", now Outlet, I just fixed that as well in the code). It defines the final path which is opened. All of this works fine for the test. It returns main and navigates there. If one runs the inner code as single app all also looks correct, will investigate further, may be just a missing sleep.

UPDATE: Just verified the code, there is a check for the browser URL at the end of the check though the behavior currently does/shall just influence the internal routing defining "Which page shall be actually rendered", in theory it can even return content w/o an URL at all. For everything else the user can/should use ui.navigate.to. I added a second unit test, one with real forwarding, one with internal forwarding. Both tests work now.

  1. Why is the instance of the SinglePageRouter in outlet called content? Would not router be a better name?

Becauuuuuuuse of this author ;-)....

image

Just with the difference that we have an additional class layer in between now, but thats the origin. Of course we can rename it if desired.

  1. The overwrite parameter in outlet.setup_page looks like a code smell -- can you add a test where it is required to be set to True? That would help me to understand the requirements better.

I removed it, it actually had no effect anymore anyway as it continued at the end of the for-loop. There is still a bug though we need to keep in mind that if you actually really share a base path, e.g. ui.page(/my_target) is defined first and then an SPA is defined at / not all functions respect this yet such as navigate.to and rather than doing a real browser navigation they try to load the page as Outlet view.

  1. Could you rename SinglePageRouter.current_router to SinglePageRouter.current_frame? Also I think there is a mix of responsibilities between SinglePageRouter and RouterFrame. Why not call it simply Frame and let it only provide the UI slot which contains sub-outlets/views?

Sure.

  1. You already explained the user_data concept a bit in Introduce API to build Single Page Applications (SPAs) #2811 (comment). But I still don't fully get it. Do you have some examples? Why can we not do it in a similar manner as with app.storage.tab?

I think the name user_data might be an unlucky / misleading choice.
What it does in a nutshell is basically transporting local variables from the outlet to the views. Our from outlets to suboutlets.
This can be references to ui.elements, but obviously also other per-user session data, e.g. caches etc.
The complex example uses is for the menu_drawer but see example below:

@ui.page('/')
def my_page()
   header = ui.header()
   ...
   def my_sub_func():
      ... can access header as its a local var ...

@ui.outet('/')
def my_outlet()
    header = ui.header()
    yield {"header": header}
    
@my_outlet.view()
def my_other_function_which_cannot_access_the_local_variables_anymore(header):
    header.hide_show_what_ever()

As it refers living / arbritrary objects it can not be serealized. It could in theory me merged with the client_storage though, the lifetime is the same as both are connection bound.

Also I still think we can simplify the API by allowing ui.outlet to have no yield statement (see #2811 (comment)). My test was quite successful when adding the following block to Outlet.build_page_template after getting the frame:

if inspect.iscoroutine(frame):
    frame = await frame
if frame is None:
    return  # if outlet builder is not a generator, run_safe already added all content

Didnt work for me back then for several functions of the NiceCloud app, would need to be investigated, the inner functions failed then. Actually I still like the clean way of definitions between "this is a template" and "this is the actual content we store in the frame".

@Alyxion
Copy link
Contributor Author

Alyxion commented Feb 10, 2025

And some more question/remarks:

  1. Do we need the SinglePageTarget.valid property? I think Outlet.resolve_target should simply return None if it does not matches the routes.

After reviewing the code: Actually its always True if either a builder was assigned and/or the fragment.
Thus yes, we can convert this to a function or an actual @property.

  1. Can we get rid of the route matching in SinglePageTarget and completely do it in the `Outlet? It would better suit their responsibilities.

Not for more complex applications, it can e.g. individualize the title, the fragment etc, the builder being called when using on_navigate. The name could / should be updated though and in general a cleanup might not hurt before merging it into main.

  1. Do we really need await ui.context.client.connected(30.0) # to ensure storage.tab and storage.client availability in Outlet.setup_page?

I have to reinvestigate. This PR is so aged mean-while but I think back then there we side effects if the client was not connected yet.

  1. Can you list all the features you implemented? Or even better: write up the tests to ensure the features are working? In dc6185d I started with the obvious ones. But there is so much more. Path matching, user_data, setting titles, ... while I was looking through the code I could barely change anything without fear of breaking it.

Yes, of course. Can not promise though when yet, quite intense releases ongoing atm and then two weeks offroad in the US, but for sure till end of March.

Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

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

I fixed some formatting and made code compatible with our ruff rules. Also I looked through the tests and added comments/suggestions here and there.

screen.wait(1.0)
screen.should_contain('main layout')
screen.should_contain('main content')
assert screen.selenium.current_url.split('/')[-1] == '' # just the content should have changed, not the url
Copy link
Member

Choose a reason for hiding this comment

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

I would expect the url path to end with /main if outlet should navigate to /main. Why is it build that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thats the concept between "builder routing" vs "real routing".

With "builder routing" like in this test you can provide ... e.g. a 404 page ... auth failed, db error, what ever ... for several endpoints. The on_navigate defines here which builder to use, w/o altering the URL.

If the URL shall be altered ... see example directly below ... it uses ui.navigate.to.

Copy link
Member

Choose a reason for hiding this comment

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

It's strange to me that this "builder routing" is done via paths. Maybe instead of an "on_navigate" function hook, we should do it more like the Starlet middlewares: a separate class you can register which can alter what is called/retuned.

def handle_navigate(url: str):
if url == '/':
ui.navigate.to('/main')
return None # prevent providing new content, forward to the new url
Copy link
Member

Choose a reason for hiding this comment

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

This looks strange. Returning None prevents navigation according to the docs. But the ui.navigate.to sill does a navigation. Why not return '/main' as string to do the navigation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See remark above, its "internal builder routing" vs "real forwarding / changing the URL".

When this function is called what will appear in the browser is already final, in on_navigate its all about what new content to show. Returning here main would thus let the URL in the browser stay at / while just changing the content. Thats not the intention here though but actually to forward the user from a generic landing page to the actual main page.

Copy link
Member

Choose a reason for hiding this comment

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

This is not "nice" enough for "NiceGUI" in my opinion. It feels like there are to many concepts intermingled. I'll think about it more...

screen.should_contain('main content')


def test_excluded_paths(screen: Screen):
Copy link
Member

Choose a reason for hiding this comment

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

How is this test different than the above? I do not see why /allowed would change the behavior of the page/outlet mixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean test_path_conflict? This one tests frame.js, the one above does not.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I still not see it. The test looks very similar. How and where does it test fame.js? Can we make the test more obvious?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh. After understanding the two navigation strategies from test_navigating_in_outlet_hierarchy. Can we merge test_excluded_paths and test_path_conflict with a similar @pytest.mark.parametrize('navigation_strategy', ['backend', 'frontend'])?

@Alyxion
Copy link
Contributor Author

Alyxion commented Feb 22, 2025

I fixed some formatting and made code compatible with our ruff rules. Also I looked through the tests and added comments/suggestions here and there.

Thank you for the optimizations, its taking shape.

As you saw I massively simplified Frame to move nearly all logic to a single location in the SinglePageRouter.

Other than that there were still some routing issues between different outlets, within outlets hierarchies etc. I fixed and wrote tests to ensure it stays that way.

Other than that there are still two missing feautures on my todo list:

  • Nested outlets can not use async yet. As all other combinations can this should be possible as well before merging this.
  • There are situations when you can already see in the Outlet than building the view can't succeed, e.g. you already detect the db is down, user is not authed etc. etc. In such siuations it might definitely make sense to be able to block all views / dont try to build them. Either via an Exception or e.g. returning rather than yielding.

One thing is also that I quite regularly see the wait for the client.connected to timeout. It does not have any negative impact and I also know for sure that the client connected successfully within this time as I need to receive its data etc. - this make it especially odd. If you have any idea what might lead to this this were appreciated.

I also btw once more reviewed your question above regarding user_data and yielding vs for example just storing it in the client storage. The major difference is that these variables are always bound to the context to which they are created, so the SinglePageRouter.

So e.g you have main_outlet.sub_outlet and yield something in suboutlet it can get automatically garbage collected once navigating to main_outlet.other_sub_outlet as the SinglePageRouter of the sub outlet is released and so all content of the user_data. So basically acting like function variables passed to nested functions.... w/o the need to do so and write cleaner, oop versions.

Some intense weeks ahead, so beyond major bugfixes I will likely continue on this end of March, doing the last refinemenets and a summary of the classes and changes involved as discussed above.

@rodja
Copy link
Member

rodja commented Feb 22, 2025

I started a checklist of open topics and todos in the description of this PR. That way we can better track what we still need to do.

@rodja
Copy link
Member

rodja commented Feb 23, 2025

I created Alyxion#4 which merges Outlet and View in favour of a unified Content API.

@Alyxion
Copy link
Contributor Author

Alyxion commented Feb 23, 2025

I created Alyxion#4 which merges Outlet and View in favour of a unified Content API.

Thanks for the changes. As written above already I'm quite skeptical about unifying the name completely - though I like the possibility allowing not to yield, e.g. in the just mentioned example that the outlet already decides / detects, that further nesting shall anyway be blocked b/c of a not available database or similar situations.

We have a quite large app meanwhile, 3 outlets, dozens of views where the outlet always also defines some kind of "gate" - ensuring oauth, ensuring DBs, initiating per-user instance variables etc, preparing shared ui elements and at least for myself I can say that having here... also in the source code... a clear visual differentiation, being able to search for all outlets in the code etc. feels cleaner than just naming them all the same - even though they effectively very different roles in practice.

@rodja
Copy link
Member

rodja commented Feb 24, 2025

As written above already I'm quite skeptical about unifying the name completely [...]: a clear visual differentiation, being able to search for all outlets in the code etc. feels cleaner than just naming them all the same - even though they effectively very different roles in practice.

The roles are not so different. Both hold UI elements, both contain "layout". The only difference is that the leafs do not contain sub-content which can be changed via routes. If your app is big and you need that differentiation, you can always introduce your own function naming scheme -- or structure with submodules. I think the API should be as simple as possible and not enforce to many concepts. Especially for new users.

[...] where the outlet always also defines some kind of "gate" - ensuring oauth, ensuring DBs [...]

In my understanding, this is a different topic and has nothing to do with Outlet/View vs Content. In the checklist I named it "rethink architecture of 'builder routing' as opposite to 'real routing' via backend". I would like to try using an approach like FastAPI/Starlette Middlewares instead of on_navigate parameter -- but that is still a few weeks in the future I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants