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

using hierarchical path names to identify JS dependencies #1134

Merged
merged 25 commits into from
Jul 11, 2023
Merged

Conversation

rodja
Copy link
Member

@rodja rodja commented Jul 6, 2023

ToDos

  • fix importmap to remove mjs warnings
  • adapt examples (custom vue component etc)
  • make tests pass again
  • define dependencies while subclassing?
  • timing issue when asynchronously loading dependencies
  • Check Path('foo/bar') on Windows
  • convert element.components into element.component
  • use ID instead of relative path for identifying dependency
  • new pytests for adding elements with dependencies on button click
  • new pytest for adding AG Grid on button click and selecting rows
  • globbing?

@rodja rodja added the enhancement New feature or request label Jul 6, 2023
@rodja rodja added this to the 1.3.0 milestone Jul 6, 2023
@rodja rodja requested a review from falkoschindler July 6, 2023 13:00
@falkoschindler
Copy link
Contributor

I've spent quite some time with the dependency registration (again). Unfortunately there are still problems. E.g. the first AG Grid demo doesn't update the age when clicking "Update" for the first time. Additional clicks do work though. Maybe that's the main reason why many tests are red. Or maybe there are more problems. I don't know at the moment.

Apart from that I'm still not very pleased with the API for registering dependencies. As soon as we leave the NiceGUI library and register a custom component in one of our examples, the base_path needs to be defined explicitly, which is rather confusing. And the amount of code needed is unpleasantly huge: register the component, register libraries, use the component in the initializer, use the libraries in the initializer, set the correct tag name.

I just spiked with passing arguments when subclassing a UI element:

class Image(SourceElement, component='image.js'):
    ...

My plan was that the directory of the subclassing class (Image in this case) is automatically the base path for the dependency. component would accept either Path or str arguments. Since strings are allowed, you don't need to import pathlib just for this line. An absolute path would be fine too, but I'm not sure what would be the base path in this case.

The subclassing method might look something like this:

    def __init_subclass__(cls, *,
                          component: Union[str, Path, None] = None,
                          libraries: List[Union[str, Path]] = [],
                          exposed_libraries: List[Union[str, Path]] = []) -> None:
        super().__init_subclass__()
        base_path = Path(inspect.getfile(cls)).parent
        cls.components = [register_vue_component(Path(component), base_path)] if component else []
        cls.libraries = [register_library(Path(library), base_path) for library in libraries]
        [register_library(Path(library), base_path, exposed=True) for library in exposed_libraries]

When instantiating the subclass, components and libraries are already assigned. Furthermore, the tag attribute will become optional, because the base initializer can pull the tag from the assigned component. In principle this is already working.

But what to do with ui.chart? There we need to distinguish between libraries that should be registered and those that should only be registered is used by an instance via extras. Maybe we need another parameter besides component, libraries and exposed_libraries: optional_libraries? Could work.

Long story short: I've got the feeling that we're on the brink of finding a really nice API. I'd like to think about it once again on Monday. Since it involves breaking API changes anyway, we should wait with the 1.3 release until this is resolved.

@falkoschindler
Copy link
Contributor

Yes, I think the libraries for ui.chart could be handled in another parameter. So we'd have

  • component
  • libraries
  • exposed_libraries (will be registered via importmap)
  • extra_libraries (will be registered but need to be explicitly used via element.use_library)

Furthermore we could support globbing:

class Mermaid(ContentElement,
              component='mermaid.js',
              exposed_libraries=['lib/mermaid/mermaid.esm.min.mjs'],
              extra_libraries=['lib/mermaid/*.js']):
    ...

@falkoschindler falkoschindler self-assigned this Jul 8, 2023
@falkoschindler
Copy link
Contributor

There's still one problem to be tackled:

def go():
    grid = ui.aggrid({'columnDefs': [{'field': 'name'}], 'rowData': [{'name': 'Alice'}]})
    grid.call_api_method('selectAll')

ui.button('Go', on_click=go)

When pressing the button for the first time, the frontend loads the AG Grid dependency asynchronously before actually rendering the element. In the meantime the API call is ineffective and no rows are selected. The second click works though.
Should we load dependencies synchronously?

@falkoschindler falkoschindler marked this pull request as ready for review July 11, 2023 15:50
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.

Ok, I think we're finally ready to merge. Today I struggled a bit with another Mermaid bug (not caused by this PR but already present in the 1.3 branch), but I could fix it with a queue in mermaid.js handling async processes one at a time.

Now the dependency API is really nice, supports globbing and works with hashed paths behind the scenes.

@rodja rodja merged commit 7a69519 into v1.3 Jul 11, 2023
6 checks passed
@rodja rodja deleted the path_improvements branch July 11, 2023 19:37
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.

2 participants