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

New event handling #1240

Closed
wants to merge 33 commits into from
Closed

Conversation

marimeireles
Copy link
Member

Collaboration from @FabioRosado @JeffersGlass @mchilvers and I on events :)
A bit messy, will start cleaning it up and adhering to what's being discussed here. Thanks all!

@marimeireles marimeireles force-pushed the events-demo branch 2 times, most recently from 3a1c4ec to 941245b Compare March 6, 2023 16:41
const isCallable = pyCallable(evalResult)

if (isCallable) {
const pyInspectModule = interpreter.interface.pyimport('inspect')
Copy link
Member Author

Choose a reason for hiding this comment

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

@antocuni this is what I need the interface for

Copy link
Contributor

Choose a reason for hiding this comment

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

if I understand it correctly, here you are trying to detect whether the user provided something which is not a callable to show a better error message, aren't you?

If so, I propose the following plan:

  1. for now, you should be able to fix it using interpreter._remote.interface. But note that when you use the RemoteInterpreter everything is async, so I suspect you will have to put await in front of every line. This should at least be able to unblock you

  2. for later, we might want to move this logic directly to RemoteInterpreter: e.g., RemoteInterpreter could have a method called isPyCallable() which checks whether the given name is actually a callable, and a corresponding method on InterpreterClient. But I don't really understand why you need to inject the evt argument so I want to understand it more

Copy link
Member Author

Choose a reason for hiding this comment

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

it worked without await 💃

@JeffersGlass
Copy link
Member

For what it's worth, pys-onClick and pys-onKeyDown were deprecated in 2022.09.1, so they've had 3 releases of being deprecated now... might just be time to remove them. If that helps clean things up at all - if no no worries, we can always do it as a separate PR.

@FabioRosado
Copy link
Contributor

For what it's worth, pys-onClick and pys-onKeyDown were deprecated in 2022.09.1, so they've had 3 releases of being deprecated now... might just be time to remove them. If that helps clean things up at all - if no no worries, we can always do it as a separate PR.

Yeah let's kill those 😄

'volumechange',
'waiting',
'toggle',
);

/** Initialize all elements with py-* handlers attributes */
export async function initHandlers(interpreter: InterpreterClient) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't personally care about other linters errors but this one in particular seems to be reasonable ... why is this callback async if there's not a single await in it?

Let's remember that if consumer code does await initHandlers(...) works regardless the callback is a Promise (async) or not, so this async feels a bit off to me ... what do you think? 🤔

@marimeireles marimeireles marked this pull request as ready for review April 12, 2023 09:41
@marimeireles
Copy link
Member Author

marimeireles commented Apr 12, 2023

Ok so, as of last week I've joined this code to the one in the main after @madhur-tandon and @hoodmane's efforts on converting everything to async/await. However I'm very confused about where to apply these changes... A lot of stuff seems to be broken... And I don't really understand why. So I'd like some help from folks:

Currently the first three tests are passing, these tests are independent of the JS side, here's an example of one of them, you can look at the rest in the file test_event_handler.py:

    def test_decorator_click_event(self):
        self.pyscript_run(
            """
            <button id="foo_id">foo_button</button>
            <py-script>
                from pyscript import when
                @when("click", id="foo_id")
                def foo(evt):
                    print(f"I've clicked {evt.target} with id {evt.target.id}")
            </py-script>
        """
        )
        self.page.locator("text=foo_button").click()
        console_text = self.console.all.lines
        assert "I've clicked [object HTMLButtonElement] with id foo_id" in console_text

Ok so this makes it very clear that the problem is happening in the JS side. Specifically in the function I've changed here in this PR createElementsWithEventListeners on pyscript.ts. My thinking that this is related to the async changes is because before merging everything worked fine, after the merge it stopped working, but I could be wrong (I'm guessing other things changed too). I've also inspected some process and they're obviously not complete (or awaited), I'm not sure how to fix it, I've tried the obvious of adding an await in the lines that I thought were relevant.

Insights here are +++

@JeffersGlass
Copy link
Member

JeffersGlass commented Apr 16, 2023

Have been doing some investigation on this this afternoon, and have a mostly-working fork of this PR. 8 of 9 tests pass. Working on cleaning it up in a way that makes it mergeable to this PR.

The one that fails is test_py_click_method_no_decorator. From some experimentation, it seems that when you grab inspect.signature via pyimport, or even just getting back a reference to it from runPython, it stops recognizing that the 'self' parameter shouldn't be counted in the paramters for bound methods. Similarly, inspect.ismethod stops recognizing bound methods. I've opened a bug report in Pyodide #3776. Update: not a Pyodide problem, see that issue.

For the time being, we might have to either get more tricky with how we identify instance methods or, what I'd suggest to get this merged, is just say the instance methods don't work yet and do it as a separate PR. 😅

@JeffersGlass
Copy link
Member

Update - the fork is at jeffersglass/events-demo-jg-contrib. There's only two extra commits on top of this PR - the first is just merging from main after the latest big Worker PR, the second is the actual event tweaks. Feel free to merge from that PR if it's helpful, or pick and choose from that second commit.

@FabioRosado
Copy link
Contributor

Nice! I'm with you it would be nice to get this in before PyCon since events are very weird to work with at the moment

@hoodmane
Copy link
Contributor

when you grab inspect.signature via pyimport, or even just getting back a reference to it from runPython, it stops recognizing that the 'self' parameter shouldn't be counted in the paramters for bound methods. Similarly, inspect.ismethod stops recognizing bound methods

I was unable to reproduce either of these issues... Maybe I am not understanding something correctly.

@JeffersGlass
Copy link
Member

JeffersGlass commented Apr 17, 2023

I was unable to reproduce either of these issues... Maybe I am not understanding something correctly.

Turned out to not be a purely Pyodide issue as I thought/put in that Issue, sorry about that. But it does still seem to be an issue with something we're doing here. The simplest way to see it (and I will try to get a minimal example together as well) is:

  1. Clone and build the fork of this PR at https://github.com/JeffersGlass/pyscript.git, branch events-demo-jg-contrib
  2. In tests/integration/test_event_handler.py, un-xfail the test_py_click_method_no_decorator test, which tests using instance methods as event-handlers
  3. Run that test
  4. The test fails with the error: UserError: (PY0000): 'py-[event]' take 0 or 1 arguments

Inside the <py-script> tag of that test, you can add the following, which shows the bound method as having 1 parameter:

import inspect
print(f"{len(inspect.signature(instance.someEventFunc).parameters)= }")

But In pyscript.ts line 195, you can add a console.log(num_params) to see that the signature claims to have two parameters, which is what's causing that UserError.

It's surely possible I'm misunderstanding something about how Synclink/Pyodide is proxying things here...

@JeffersGlass
Copy link
Member

JeffersGlass commented Apr 17, 2023

Hmmmm no it seems either I'm wrong again or I'm misunderstanding how we're using Synclink in PyScript. I set up a minimal synclink+pyodide example, and in that instance it seems inspect.signature has no problem identifying the number of parameters:

pyodide.asm.js:10 This got 1 argument: foo
pyodide.asm.js:10 len(inspect.signature(instance.someMethod).parameters)= 1 //This is in Python
main.ts:31 Number of Parameters: 1 //This was called using pyimport('inspect')
main.ts:34 User code complete

So, probably not an upstream bug an all, but something I'm not understanding about our new setup. On the other hand, I've learned quite a bit about Synclink in the last day, which is nice.

@FabioRosado
Copy link
Contributor

Just to confirm, Jeff are you blocked by that bug you reported?

This PR has been open for a while and I think we should improve event handling 😄

@JeffersGlass
Copy link
Member

@FabioRosado Not blocked, just need to find the time to actually identify what's wrong in the current code and make the fix. I haven't had much time to spare on it in that past week, with PyConUS and all. But agreed - would love to have this in!

With circumstances being what they are, I will probably end up closing this PR and opening a fresh one on a branch I can push to. Perhaps I'll do that now.

@FabioRosado
Copy link
Contributor

Sounds good didn't want to rush just wanted to check if you wanted me to try and take it over although I am completely out of the loop with all the awesome work 😄

Yeah that's probably a good idea

@WebReflection
Copy link
Contributor

Hello folks 👋

With the landing of #1403 this discussion looks like a piece of cake to me ... beside the MutationObserver story which is something easier to implement now, but I think it should be a PR a part after this is solved/fixed/landed.

To me the little changes to apply to current code is:

  • if type.endsWith('-code') in the event attaching loop, we use an eval() logic instead, but I am not sure I understand what that means ... maybe reading this PR would tell
  • some little check on the provided code should be enough ... as example, if there's any = or ( char in the event we can show the warning but imho we shouldn't put too much effort into that ... it's explicit enough already to decide if an event should evaluate code or just invoke a globally exposed callback

If my understanding is right, and since nobody is assigned to this issue, I'd be more than happy to address this part and improve the recently already improved logic, covering 80% of the proposal goal.

Thoughts?

@FabioRosado
Copy link
Contributor

Hello folks 👋

With the landing of #1403 this discussion looks like a piece of cake to me ... beside the MutationObserver story which is something easier to implement now, but I think it should be a PR a part after this is solved/fixed/landed.

To me the little changes to apply to current code is:

  • if type.endsWith('-code') in the event attaching loop, we use an eval() logic instead, but I am not sure I understand what that means ... maybe reading this PR would tell
  • some little check on the provided code should be enough ... as example, if there's any = or ( char in the event we can show the warning but imho we shouldn't put too much effort into that ... it's explicit enough already to decide if an event should evaluate code or just invoke a globally exposed callback

If my understanding is right, and since nobody is assigned to this issue, I'd be more than happy to address this part and improve the recently already improved logic, covering 80% of the proposal goal.

Thoughts?

Just to be sure, does that mean the whole work with the @when decorator as well? Or just handling -code cases?

This is just an opinion, but if the use case of -code seems to make things a bit harder, how about we try to merge the @when decorator without the -code use-case and then do a follow up PR to support it?

@WebReflection
Copy link
Contributor

WebReflection commented Apr 27, 2023

Just to be sure, does that mean the whole work with the @when decorator as well? Or just handling -code cases?

This is just an opinion, but if the use case of -code seems to make things a bit harder, how about we try to merge the @when decorator without the -code use-case and then do a follow up PR to support it?

right, admittedly I wrote my comment before reading (or understanding) the current PR or considering the @when decorator case.

as it looks like I need way more background before jumping on this issue or try to solve it, I am keen to make a step back at least until I fully understand the whole story.

Apologies for the rushed comment, happy to answer any question around current simplified events dance ... the most important thing is that we can literally check any py-* attribute in one shot and also understand if *-code is in there and branch out the logic when that's the case.

@JeffersGlass
Copy link
Member

Hi y'all - currently working on breaking out the @when decorator into its own PR, since it's a bit of a separate case from the py-[event] and py-[event]-code syntax.

Will have that shortly, and once it's up, I think we should close this PR and break out a fresh one for the changes to the HTML-attribute-based event handling. @WebReflection you may have found it already, but that syntax is described in #1222.

This was referenced Apr 27, 2023
@JeffersGlass
Copy link
Member

To move this forward, and since #1403 introduced quite a few merge conflicts/improvements to main, I'm going to close this PR in favor of two new ones:

Some really excellent work and discussion were had here, and I don't want to discount or lose any of that. Mostly making the split for logistical reasons and to allow us to get the great parts of this PR merged into main soon. 😁

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.

None yet

6 participants