Skip to content

Assert on queriedButton not button in the test #6

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

Merged
merged 1 commit into from
Dec 27, 2020
Merged

Conversation

MaxDesiatov
Copy link
Contributor

The current test was meaningless as we didn't assert on the text content of queriedButton.

I've also removed addEventListener because it doesn't actually work. The button instance is already deallocated by the time that test has finished, so the closure passed to addEventListener is released too. We need to find a way to run async tests like this, and I have a few thoughts, but this still needs to be explored.

@MaxDesiatov MaxDesiatov requested a review from a team December 27, 2020 20:10
@j-f1
Copy link
Member

j-f1 commented Dec 27, 2020

Maybe we could stash the event callbacks in a global registry? That’s a simpler case than storing arbitrary functions since we know to remove them from the registry when calling removeEventListener (or watching for the node to be removed from the DOM?) Perhaps we can be OK with some level of memory leakage by default to provide good UX, as long as we document it.

@MaxDesiatov
Copy link
Contributor Author

What if removeEventListener was never called? The only thing we can rely on is a call of deinit on a corresponding element, which is what we're doing right now. JavaScript won't let us know when exactly that element is gone.

I'm still convinced that the whole closure deallocation problem can be resolved with Wasm reference types as I described in swiftwasm/JavaScriptKit#106. At least reference types are already being worked on in Safari, while I'm not sure if they'll even consider implementing FinalizationRegistry.

@MaxDesiatov MaxDesiatov merged commit e45d37f into main Dec 27, 2020
@MaxDesiatov MaxDesiatov deleted the fix-test branch December 27, 2020 21:35
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