Skip to content

Create proxy fix #3345

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Create proxy fix #3345

wants to merge 6 commits into from

Conversation

mEp3ii2
Copy link
Contributor

@mEp3ii2 mEp3ii2 commented Apr 18, 2025

Improve event binding across web back-end by replacing the old create_proxy() with the pyodide.ffi.wrappers.add_event_listener(), this has been done in the libs module.
This affects any web widgets using DOM events such as blur, focus, change etc
Changes has been made to the passwordinput widget as well as the window module to make use of this change

New usage is now
create_proxy(dom_element,"event-type",python_handler)
e.g.
create_proxy(self.native,"sl-blur",self.dom_onblur)

Resolves issues related to garbage-collected borrowed proxies that show in the console.

Background
Brought to my attention in my previous pull request

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The fix you've provided here looks like it resolves the issue; however, I'm not completely convinced it's the right approach.

Taking the solution "as is" - I'm not wild about the name change. You've taken a method called addEventListener and rebound it to the name create_proxy - which is a method name that also exists in the pyscript.ffi namespace. That means create_proxy() is (a) no longer doing what the built-in create_proxy method does, and (b) doesn't accurately describe what it does do, either. If we were going down this path, I'd be inclined to make a utility add_event_listener() method (possibly as member method on the widget class).

However, I'm not completely convinced this is needed. The ffi methods you're using here are low level pyodide methods; and one of the goals of PyScript is to provide a higher-level API that abstracts those methods and details behind a more Pythonic API.

To that end - Pyscript even provides a wrapper around create_proxy - but immediately advises that it shouldn't ever be needed if you're using the pyscript API as intended.

In particiular, the pyscript.web.* APIs (additional user guide here) provide a way to create any DOM element, and presumably it also "does the right thing" when it comes to attaching handlers - that would seem to be the intention of the when() method/decorator.

So - can we avoid the need to call pyodide internal APIs entirely by using the pyscript API exclusively? Can we replace Widget._create_dom_element with a native Pyscript call?

Briefcase is currently using PyScript 2024.11.1; if we need to switch to a newer version of PyScript to get bug fixes and updates, then that's absolutely something we can and should do. This is also something that would fit into the bigger piece of work of making Briefcase indepdendent of the specific needs of Toga - that is, the Pyscript and Shoelace version dependencies are dependencies of Toga, and should be defined there, not defined as part of Briefcase.

One other detail - when you're proposing a change that is purely code, and doesn't contain a change to tests, it's worth flagging how you've tested this yourself. If the code has tests (or a pre-existing, but failing test), then the "yes, this works" criteria is obvious; but when it's just code change, it's not necessarily obvious how you've established that code is working.

In this case, my go-to was the passwordinput example in the examples folder... and it won't work because (a) it's currently missing a travertino dependency, and (b) the dom_onchange implementation on PasswordInput is currently passing in a None argument whenever it is fired. You might not have hit the former problem (addressed in #3346) if you rolled your own test case - but I'm not sure how you could have tested any change event handling on PasswordInput and not hit the second problem.

mEp3ii2 added 2 commits April 20, 2025 20:50
Grabbing changes from upstream
@mEp3ii2
Copy link
Contributor Author

mEp3ii2 commented Apr 22, 2025

Briefcase is currently using PyScript 2024.11.1; if we need to switch to a newer version of PyScript to get bug fixes and updates, then that's absolutely something we can and should do. This is also something that would fit into the bigger piece of work of making Briefcase indepdendent of the specific needs of Toga - that is, the Pyscript and Shoelace version dependencies are dependencies of Toga, and should be defined there, not defined as part of Briefcase.

How would you suggest going about this. I'm assuming it would require coordinating PRs across both Toga and Briefcase to achieve this. Just want to make sure that I've understood this correctly. Also any tips or good to know things before i start would be greatly appreciated

@freakboy3742
Copy link
Member

How would you suggest going about this. I'm assuming it would require coordinating PRs across both Toga and Briefcase to achieve this. Just want to make sure that I've understood this correctly. Also any tips or good to know things before i start would be greatly appreciated

When Toga uses Briefcase, it's always using the "development" version; so as soon as any changes are made upstream, those changes will be used by Toga. And since the testing story for the web backend is weak, there's no chicken-and-egg coordination problem - there's very few automated tests that need to pass before we can merge changes to the web template, so we don't need to worry about fixing Toga so that the template change will pass automated testing so that we can merge a PR so that Toga can use new features :-)

The two possible places a change might be required are in Briefcase itself, and in https://github.com/beeware/briefcase-web-static-template - the template that is used to generate Web code.

The minimalist fix will be to update (2) to point at the newly required version. Assuming there's no other large configuration changes, this may be all you need to do. If you want to test things out, you can clone the template repository, then add template = "/path/to/template/checkout" in your pyproject.toml in the web configuration section, and Briefcase will use your local checkout. Modify the Pyscript version referenced in that template, test it, submit a PR, and you're essentially done.

The intermediate fix would be to decouple the PyScript version from Briefcase entirely. To do this, you'd modify the web template to make the Pyscript version a template variable, and use that template variable in the template (with a value of 2024.11.1 as a default, so that existing apps don't see a change in behavior). You then document in Briefcase that you can use pyscript_version = "X.Y.Z" as a Briefcase configuration variable, and modify the Briefcase Toga bootstrap to specify the new version of PyScript. This relies on the fact that any configuration item in pyproject.toml is passed down to the template as context - so you don't actually need to do anything in Briefcase to expose a new configuration item for a template to use.

The comprehensive fix would be to resurrect the parts of #1945 and beeware/briefcase#1285 that would allow Toga to specify the version of Pyscript (and any other dependencies, such as Shoelace) that it requires. Those PRs allow a wheel to specify "inserts" which will be processed by Briefcase, and inserted into index.html at locations that the template allows. The inserts defined in the PR only include Shoelace includes and a verbatim block of CSS... but it would make sense to also include the Pyscript version definition, so that the Pyscript version is bound to Toga, not Briefcase - avoiding this problem completely in future. This might require some design work on how that version specification would be declared.

In terms of a pragmatic approach, I'd be entirely comfortable with the minimalist approach right now, as that gets you unstuck on the immediate issue of updating Pyscript API usage.

However, I'd like to think you and the team would be able to take a swing at the intermediate or comprehensive fix as part of the work that you're doing improving BeeWare's web tooling.

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

Successfully merging this pull request may close these issues.

2 participants