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

Improve javascript execution #112

Closed
rodja opened this issue Oct 8, 2022 · 3 comments · Fixed by #115
Closed

Improve javascript execution #112

rodja opened this issue Oct 8, 2022 · 3 comments · Fixed by #115
Assignees
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@rodja
Copy link
Member

rodja commented Oct 8, 2022

I think we have two issues with the current javascript execution, which I put into one issue because of their interlocking:

1: unintuitive naming

We have two separate calls: ui.run_javascript to just execute and ui.await_javascript to get the value back. Both are awaitable. So the distinction is not self explaining. And writing my_value = await ui.await_javascript(...) is not nice.

I suggest we merge both features into a single ui.run_javascript call. It always returns the data from the executed javascript which can also be None.

2: silent ignore before page is ready

Calling ui.run_javascript before the page is ready is silently ignored. We could raise an Exception in this cases. But I'd rather make the javascript execute directly after page creation.

rodja added a commit that referenced this issue Oct 8, 2022
rodja added a commit that referenced this issue Oct 8, 2022
rodja added a commit that referenced this issue Oct 8, 2022
This code lead to the creation of #112.
The use of on_page_ready callback is not "nice".
@rodja
Copy link
Member Author

rodja commented Oct 8, 2022

In 18b390c I made ui.run_javascript waiting for the page to become ready. This worked well for scenarios like when a value_changed is triggered for a selection:

radio = ui.radio(['Page Title A', 'Page Title B'], on_change=set_title)
# NOTE setting the value here will trigger on_change before the page is opened, so js needs to hold back until the page is loaded
radio.value = 'Initial Page Title'

But on an async page, this could easily create a deadlock:

@ui.page('/')
    async def page():
        ui.label('before js')
        await ui.run_javascript('document.title = "A New Title"')
        ui.label('after js')

Therefore I changed the behaviour to raise an exception if javascript is requested to execute before the page is ready.

@rodja
Copy link
Member Author

rodja commented Oct 8, 2022

The code to run JavaScript immediately after page is finished is not very nice. Due to the page handler we need to make internal objects available to the outside. See the map example:

locations = {
(52.5200, 13.4049): 'Berlin',
(40.7306, -74.0060): 'New York',
(39.9042, 116.4074): 'Beijing',
(35.6895, 139.6917): 'Tokyo',
}
selection = None
@ui.page('/', on_page_ready=lambda: selection.set_value(next(iter(locations))))
def main_page():
# NOTE we need to use the on_page_ready event to make sure the page is loaded before we execute javascript
global selection
map = leaflet.map()
selection = ui.select(locations, on_change=map.set_location).style('width: 10em')

Maybe the page builder could also be a generator. All code after yield is executed after the page is ready. A similar technique is used in fixures from pytest: the tear down code is executed after the yield statement.

@rodja
Copy link
Member Author

rodja commented Oct 8, 2022

I found a way to use the yield keyword and created pull request #114. With this the example becomes much nicer:

@ui.page('/')
def main_page():
map = leaflet.map()
locations = {
(52.5200, 13.4049): 'Berlin',
(40.7306, -74.0060): 'New York',
(39.9042, 116.4074): 'Beijing',
(35.6895, 139.6917): 'Tokyo',
}
selection = ui.select(locations, on_change=map.set_location).style('width: 10em')
yield # all code below is executed after page is ready
default_location = next(iter(locations))
# this will trigger the map.set_location event; which is js and must be run after page is ready
selection.set_value(default_location)

@rodja rodja linked a pull request Oct 8, 2022 that will close this issue
1 task
@rodja rodja removed a link to a pull request Oct 8, 2022
1 task
@rodja rodja added the enhancement New feature or request label Oct 8, 2022
@rodja rodja linked a pull request Oct 13, 2022 that will close this issue
1 task
@falkoschindler falkoschindler added the bug Something isn't working label Oct 13, 2022
@falkoschindler falkoschindler added this to the ASAP milestone Oct 13, 2022
@rodja rodja modified the milestones: ASAP, v0.9.18 Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants