-
-
Notifications
You must be signed in to change notification settings - Fork 902
Patch and warn late event registrations #5439
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
Patch and warn late event registrations #5439
Conversation
falkoschindler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request, @evnchn! I finally looked into it:
-
Input elements still loose focus. But I guess that's better than no event registration at all.
-
There's an inconsistency between Python and JavaScript: The Python warning triggers for any listener change (add/remove, same or different type), while JS only re-renders for new event types. Here is an example where this fails to register the second event handler:
i = ui.input().on('keydown', lambda e: print(e)) ui.timer(1.0, lambda: i.on('keydown', ui.notify), once=True)
-
Maybe it would be a good opportunity to change the "too_long_message" event to a more generic "warning" event. Then we could let the client create the warning for all and avoid re-implementing the detection in Python, probably fixing point 2.
-
Overall it's quite some code for an imperfect solution. But maybe thats the best we can do. With point 3 this PR will mainly take place in nicegui.js. And it's not the fault of this PR that nicegui.js is borderline unreadable with its almost 500 poorly structured lines of code. 😉
|
Actually do we really need such a big popup for re-rendering the element. Maybe just invoking the Quasar loading bar at the top of the page for a split second is enough to indicate some loading is happening. Regardless I think both is fine because either way late-registrations are not recommended. |
|
I didn't expect you to change any popups. I was just thinking about merging multiple errors/warnings into a single message topic. We probably shouldn't implement so many popups, especially for technical details like an event registration. Logging the warning should suffice for the developer to notice. Oh and tests are failing right now. |
61c7cfa to
83212dc
Compare
- Make it stricter by having another keydown, this way you must monitor the ID set - Remove the python logging assert
|
Should be good:
|
|
I just introduced a "log" message as an easy way to log warnings and errors both in JavaScript and Python. This replaces the hacky "too_long_message" message. The test is green. Here is my code for experimentation: # experiment with large payloads
ui.textarea(value='x' * (1_000_000 - 242), on_change=lambda e: ui.notify(f'{len(e.value)} characters'))
# experiment with late event registration
name = ui.input()
name.on('keydown.a', ui.notify)
ui.timer(1.0, lambda: name.on('keydown.b', ui.notify), once=True)Do you agree, @evnchn? |
evnchn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it works for the most part, this breaks polling transport.
Try it with core.app.config.socket_io_js_transports = ['polling']
Motivation
Reading #4154 and #4828 again, I come to the following conclusion:
Hence this PR.
Implementation
Code is extremely similar to #4828
But on top, I added warnings both in Python-land and JS-land.
Progress
Final notes
How I read it, #4828 was closed because:
Both of which should no longer be blockers, given the new code and new positioning on the issue.
So hopefully this gets a pass. Thanks!