Add single listener deregistration #1969
Conversation
joshbuddy
force-pushed
the
feature/single-listener-off
branch
from
3a20a54 to
ddbfa6c
Compare
| eventListeners[eventName].push(thisListener); | ||
| return thisListener; |
There was a problem hiding this comment.
probably better to return "cancel" callback instead, to avoid exposing internal Listener API
There was a problem hiding this comment.
This is the question I brought up in my issue. I can see advantages in having a more active style api, and returning the listener itself would facilitate that. Otoh, simply returning the cancel function is the 80% usecase, so I dig that simplicity.
There was a problem hiding this comment.
Returning a cancel callback was the first thing I thought of too. How easy is it to manage those callbacks though? Are the 2 approaches mutually exclusive?
There was a problem hiding this comment.
So, I think it would be better to have a design discussion on the issue instead of the PR, but happy to do it here if you're really into that.
In short, if this was my design, I would have a low level Listener interface, and then some higher level functions on top that correspond to the existing api, but with a few tweaks. I would more closely align it with nodejs's eventemitter api with a goal of it being more idiomatic js.
There was a problem hiding this comment.
And, I'm happy to make the changes to the api to more what I'm describing, just didn't want to actually do that unless people were into it.
There was a problem hiding this comment.
Returning a cancel callback was the first thing I thought of too. How easy is it to manage those callbacks though? Are the 2 approaches mutually exclusive?
I think main problem that approach with returning listener object will expose it when it is not intended to be exposed (it seems private interface with limited documentation).
joshbuddy
force-pushed
the
feature/single-listener-off
branch
from
ddbfa6c to
598d631
Compare
joshbuddy
force-pushed
the
feature/single-listener-off
branch
from
598d631 to
a9a74e3
Compare
|
This change would need an update to |
|
I think I would not touch the go api for now, merely ship this, and then revisit the go api with an eye towards making it more go-like. |
|
I think it's important to keep the API as consistent as possible between Go and JS. I think returning a function there is also ok? |
joshbuddy
force-pushed
the
feature/single-listener-off
branch
from
1f0fb2a to
0b745c2
Compare
|
Added a sample test for Go. Feel free to update with your own tests 👍 |
|
@leaanthony so, I haven't seen any place for js tests to go here. If there is I'll happily add them. If there isn't I'm going to hold off on adding tests to this pr, better to do it in a separate branch |
|
I'm happy for you to add a testing framework for JS to this PR. There isn't too much to test. I think my preference would be vitest: https://vitest.dev/ |
|
@leaanthony sure, sounds good! Any thoughts on integration tests themselves? I couldn't see any work in that direction |
|
Open to ideas. If you mean between Go and JS, then yeah... Nobody is sure yet. I've been meaning to check out https://nutjs.dev/ to see if that might help. I think we may need a "test" target that builds against a headless frontend that we can query. It's pretty tricky. |
Probably is best idea to start with unit tests for core functionality, then integration tests using browser and after that time will come to e2e tests. I did small research some time ago for my own apps; however, there is no good solution for e2e tests of webview that cover all 3 targets (Windows, MacOS, Linux). It easy for Windows and Linux as they use Chromium but MacOS will be left aside. |
|
Sounds like writing good integrations tests is a big unknown but high value if we can make them robust. Anyhoo, lets finish off this work before figuring out stuff like that. @leaanthony thanks for the vitest reco. Very pleasant test framework to use! |
|
@leaanthony I think this is ready |
|
Thanks so much for taking the time to do this! A couple of minor things and I think we should be good to go:
|
|
@leaanthony how's that look? |
|
@joshbuddy - yeah cool. I think the first link wasn't updated (same filename - easy to confuse!). |
|
@leaanthony Hey, then I'm not clear what you mean by updating https://github.com/wailsapp/wails/blob/master/v2/internal/frontend/events.go. This is the current state of that file: |
|
Oh that's so weird - I must have mistakenly been on a different branch 🤯 |
|
Thanks so much for this @joshbuddy ! |
This provides an API to address this issue.
#1962
My one question with this change is how do you test these sorts of changes?