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

Feature/dependencies #658

Merged
merged 62 commits into from
Jun 26, 2023
Merged

Conversation

dclause
Copy link
Contributor

@dclause dclause commented Mar 28, 2023

Here is a pull request that shows the restriction of dependencies per elements really used in the page.

Here is a simple example using a simple label:
image
And the same with this PR:
image

What this PR does it :

  • it renders the .vue component at the bootstrap rather than on fly (at each request). Meaning a vue file is render ones only and the time taken is moved from the page load to the application bootstrap
  • it moves the libraries in separate folder (required for ESM multi-file loading like mermaid)
  • it supports both ESM and standard libraries
  • it updates mermaid to mermaid 10 to demonstrate ESM loading

/!\ Currently only plotly and mermaid as been updated so anything else would fail, including the website.
plotly demonstrate the usage of a standard library, along with the support for SFC (.vue) file, and mermaid element is here to show the use of an ESM library.

Here is a demo file to test:

#!/usr/bin/env python3
from nicegui import ui
import plotly.graph_objects as go

ui.label('test')

fig = go.Figure(go.Scatter(x=[1, 2, 3, 4], y=[1, 2, 3, 2.5]))
fig.update_layout(margin=dict(l=0, r=0, t=0, b=0))
ui.plotly(fig).classes('w-full h-40')

ui.mermaid('''
    graph LR;
        A --> B;
        A --> C;
    ''')
ui.run(port=5555)

The PR shows a few API addition and changes quite some stuff to work, but the benefit in term of amount code loaded are good I guess. That is specially interesting in an embedded robot situation where the robot battery and poor network is at stake (and we don't have CDN support). Just try current and PR (using the test file above) by simulating a fast 3G network to see the benefits
@falkoschindler @rodja Can I have your review and opinion on the choice, eventually other way to achieve it before I migrate every single components.

Note:
.vue file needs an extra compile process that is done using VBuild. That tool is 4 or 5 years old, does not support any vue3 syntax (composition API, no import, no css-binding, etc..) The only benefit it has is to embed some CSS, honestly we could inline it and remove the .vue (SFC) syntax to only allow JSX files.
The conversion is really really easy: jus tmove the template into the template key and inline the CSS.

By doing so, we could avoid the 'expose' key in the API and the double standard ESM vs standard import. That would make the JS responsible for the import, just like mermaid does in the PR:
import mermaid from 'mermaid'; at the beginning of file.
We could also loose the use_library: a component would only use_component to load it's associated JS file, and the rest would be handled in that JS file.


Task list:

  • After merging this PR, think about Internationalization (i18n) for NiceGUI #389 (comment)
  • fix pytests for ui.aggrid
  • fix pytests for ui.mermaid
  • fix pytests for ui.plotly
  • @todo in index.html? --> maybe later
  • upgrade Mermaid to ESM (TODO in fetch_dependencies.py`)
  • upgrade Three.js to ESM (TODO in fetch_dependencies.py`)
  • remove ui.run(..., exclude=...)

@rodja
Copy link
Member

rodja commented Mar 28, 2023

Wow. That is very impressive. We will need some time to look at it. Does this approach use server-side rendering? That would be very beneficial for SEO, too. See discussion #637.

@dclause
Copy link
Contributor Author

dclause commented Mar 28, 2023

No server side rendering. Just filtering the assets per elements used in the page. It is not as much as it looks like and requires some extra API for elements (see the use_component, use_librairie) to actually link a component or library to its element.

I will wait for your review before pursuing migrating the elements because that would not be worth it if we need to change the API proposal here.

About server side rendering: I don't think SSR could be a thing here without nodeJS. That would require to reimplement Vue rendering in python otherwise.

@cj
Copy link

cj commented Mar 29, 2023

@dclause, fantastic work! This is precisely what I thought NiceGUI was missing when I found it—only loading the external libs it's using on the page instead of all of them!

@cj
Copy link

cj commented Mar 30, 2023

This is a "Hello World" test I ran locally:

1.2.2 (Current Version)
image

1.3 (This pull request by @dclause)
image

100kb of that is tailwindcss.

@dclause
Copy link
Contributor Author

dclause commented Mar 30, 2023

@cj I think the improvment in performance is uncontestable! I am waiting for @rodja and @falkoschindler technical review to check if this goes in the proper direction according to there requirements. When done, I will update every other elements.

As of tailwind, you can use tailwind=False in ui.run() to disable it, but this is flagged experimental and I don't know if that is working fine or not. I would argue that Quasar already have a bunch of CSS features so Tailwind really feels overkill here. It is even heavier than bootstrap!

A lighter yet (theoretically) compatible solution would be to use tailwind which is designed to be a 17Ko replacement for tailwind (I have never used it though, that would need to be tested)

@dclause
Copy link
Contributor Author

dclause commented Mar 30, 2023

@cj Just wanted to add that we don't have a real full vueJS ecosystem on the project, and we don't benefit for tree shaking here. If you use the chrome console "coverage" tab, you will see that most of the asset is actually not used.

For instance, taking the demo code above, even with my PR removing fully unused depedencies, we still use only part of the loaded dependencies:
image
(the red part is the unused part of the loaded code)

Maybe using destructuring import could help, but I am not sure this can be done outside a build phase like webpack does.

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

@dclause

I just review your PR. It's really impressive, not only by the diff size, but also by the simplicity of the solution. If we can get this to work, it would greatly improve the performance by simplifying the implementation.

In my "review" commit I just improved some minor code formatting or type annotation things, something I like to do when approaching know new code instead of just reading it. Here and there I could simplify things a little. For libraries I introduced a dataclass, but as you indicated it might be obsolete anyway.

There are four main concerns I have at the moment - for no reasons I hope:

  1. When trying your demo file, the UI shows only label and plot. After half a second the plot resizes and the Mermaid diagram appears. I haven't investigated further though. But, can we improve on that?

  2. When running main.py, the live demos are dead. Maybe this is not supposed to work yet, I just noticed.

  3. I can't really judge the new Mermaid implementation as it doesn't seem to be fully functional yet. In the end the example code from Mermaid in markdown #278 (comment) should work. But currently updates do not reach the client and you can't have multiple Mermaid diagrams on one page.

  4. You wrote about dropping SFC support. When transitioning to Vue for NiceGUI 1.0 I thought it would be very valuable to support .vue files. As far as I know this is the only way to propery define scoped CSS for Vue components. But since then I rarely actually used SFCs. If you say it's very easy to convert to .js, we might as well drop .vue. It would further simplify NiceGUI's dependency management. I just worry the community might miss SFCs for some reason.

To conclude: I'd love to see this PR evolve further and to eventually merge it into main. And maybe my concerns resolve once the code matures a little more.

@falkoschindler
Copy link
Contributor

Let me convert this PR into a draft to indicate it's current state.

@falkoschindler falkoschindler marked this pull request as draft March 30, 2023 17:15
@dclause
Copy link
Contributor Author

dclause commented Mar 31, 2023

Thanks @falkoschindler for this review.

  1. Due to how library / loading works in the PR, a resize event is not fired anymore in the mounted. That let me discovered that the current plotly element actually hardcodes its size to a default value and niceGUI introduced a resize listener to override this and rebuild in a wider size. Until now, the loading was done in the component itself, wait until loaded, then build and mount. That triggers a component resize, which itself force a rebuild to the full-size. In this PR, the full-size is obtained via full-width first due to tailwind, time during which plotly loads asynchronoulsy, then build at the default size. But the container having its size, it does not triggers the rebuild. After investigating the plotly API I found they have a responsive key that do not force a specific size for plotly. I could leverage that API and remove all-together all resize listeners in niceGUI which is nice. You will see in my latest commit that plotly as become really light now in it's vue file ! You can retry the demo, plotly now loads at scale directly.

For mermaid, this is another issue. I do think that it is because they now start by clearing out the container first. For sure, this causes the blink if you reload the page (F5), not sure if the initial load comes from this or the ESM loading. You will notice that the import statement is in the component js file itself, meaning it cannot start until the component as been replied by the API (as opposed to currently where all library are loaded regardless and independently from the element. I tried to move that import to the index.html but it seems that there is a scope issue. If not loaded within the vue app, I could not use the mermaid tool. I am not yet sure how to bypass that. I have reworked a bit the mermaid.js file does it makes it faster in anyway in term of initial loading time ? Will it be an issue ?

  1. I did not even expected main.py could be started at this point. All elements with js file or external library needs to be converted first until I we can expect the full website to work properly.

  2. I did completely forgot to avoid double loading !! Meaning not just mermaid, any element that would be used twice on the page would fail! Stupid me!! I have commited a fix and also fixed the mermaid component to manage the bind_content(). See this demo now:

ui.label('This is a test page!')

fig = go.Figure(go.Scatter(x=[1, 2, 3, 4], y=[1, 2, 3, 2.5]))
fig.update_layout(margin=dict(l=0, r=0, t=0, b=0))
ui.plotly(fig).classes('w-full h-40')


class Mermaid:
    content = '''
        graph LR;
            A --> B;
            A --> C;
        '''

ui.mermaid('').bind_content(Mermaid, 'content')

def update():
    Mermaid.content = '''
        graph RL;
            A --> B;
            A --> C;
        '''


ui.button('Update', on_click=update)

ui.mermaid('''
    graph LR;
        foo --> bar;
    ''')

ui.label('Want another one ?')

ui.mermaid('''
    graph RL;
        foo --> baz;
    ''')

Regarding markdown, I was not aware it could run external tools too like mermaid. This for sure is a challenge too. This one is my new worry now (along with the conditional libraries of highchart). Those are definitely two elements I have to work on now.

  1. There is no otherway around scoped css than .vue file. We don't use it (or don't need it) in niceGUI implementation, but people might use it for there own components. It it avoidable using classes but CSS is definitely more difficult to handle without the SFC component.

IMPORTANT NOTE: mermaid support ID for HTML4 which stands that it cannot be an integer value. Because other tools might have this same limitation, I changed all generated id to be prefix with 'v'.

@falkoschindler
Copy link
Contributor

Thanks for the update, @dclause!

  1. Sounds amazing! Removing quite some code while improving performance is all we can wish for. 😄

    I think the short blink when loading Mermaid elements is ok. In the current version there's something similar happening: https://nicegui.io/documentation/mermaid

  2. 👍

  3. 👍

  4. I think, as SFC support is not a huge burden, we should keep it for now.

There's one glitch happening sometimes when refreshing your code example from #658 (comment). Both "foo" diagrams land on top of each other:

Screenshot 2023-03-31 at 21 38 30

Apart from that, it looks like you're busy with Markdown and Highcharts. Let us know if you need support or when it's a good time for another code review.

@falkoschindler
Copy link
Contributor

I spent some time with this PR today in order to tick some of the open issues from my last comment. There is mainly one thing left to do: fix pytests for ui.aggrid, ui.mermaid and ui.plotly. These are the ones I couldn't figure out by myself and I think they are caused by the new dependency mechanism, but I might be wrong.

@dclause Do you think you could have a look and help finishing up this PR? We're already working on NiceGUI 1.3 and I would definitely like to have this PR being a part of it.

Besides the tests, are there other things we need to do before releasing 1.3? There's a @todo left in index.html - not sure if this is important. And we should probably upgrade to ES modules for Three.js and Mermaid (see TODO markers in fetch_dependencies.py). I'll update the task list in the original post to reflect the progress of this PR.

@dclause
Copy link
Contributor Author

dclause commented Jun 16, 2023

@falkoschindler Thanks a lot for working on the subject !
I do have important deadlines (both pro and perso) until end-july that requires most (if not all) of my time currently and I have trouble to keep up with the open-source contributions.
This topic is definitely on the top priority of what I would like to see done, so be sure that I don't forget about it, it is only a matter of time.

  • This PR already uses ESM threejs (see npm.json and npm.py files). The dependency folder structure introduced in this PR is different from what generates fetch_dependencies. You can see at the scene element we add the library using "expose" which exposes the ESM compatibility layer for this to be used.
  • I would need to refresh my mind on the subject, but the @todo in the index.html file is about the initial page load, since we already know about the libraries, I feel like a trick could be found to make that initial load better. I can't remember the reason about it on top of my head, I would need to deep dive back in the work.
  • I have no ideas about the test as of now. Last time I checked, I was not able to try the tests locally. I need to retry my setup and eventually I will ask your help for this at some point.

@falkoschindler
Copy link
Contributor

Yes! I managed to fix all three elements. It turned out to be a tiny bug when a library has the same name like the Vue component. Now the tests are green again.

So now I only have to think about fetch_dependencies.py, I guess. But not today.
And I think the @todo in index.html is fine for now. We can very well improve it later.

@falkoschindler falkoschindler added the enhancement New feature or request label Jun 23, 2023
@falkoschindler
Copy link
Contributor

Ok, it looks like this PR is ready to merge into 1.3. 🏁
All todos are completed, pytests are green (on my machine), and the dependency upgrade works.
The actual upgrade will be done right before we release 1.3.0.

@rodja rodja changed the base branch from main to v1.3 June 26, 2023 08:58
# Conflicts:
#	nicegui/elements/joystick.py
@falkoschindler falkoschindler merged commit 3b76e95 into zauberzeug:v1.3 Jun 26, 2023
@dclause
Copy link
Contributor Author

dclause commented Jun 26, 2023

<3 !
Congratulation @falkoschindler :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants