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

fix(react): cleanup events if they are no longer passed as props #148

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

Conversation

WickyNilliams
Copy link
Contributor

Say i have some (react) code like:

someCondition 
  ? <MyComponent onSomeEvent={handleSomeEvent} />
  : <MyComponent />

Then the event handler doesn’t get removed when someCondition is false.

The current code for syncing props, only considers new props when adding/removing event handlers, and doesn’t remove handlers that existed in previous props but not in new props.

This PR modifies the attachProps util to call removeEventListener on any listener which is not part of the latest props.

I had a little trouble writing a good test (read: not testing implementation details), since I think testing-library does not work well with web components. So I just followed the approach in other tests (inspecting __events) instead.

I also attempted to fix the build, as the file:// urls for the svelte output target were causing issues.

@arielsalminen
Copy link
Contributor

Same comment here, would be great to get this merged in.

@arielsalminen
Copy link
Contributor

Any updates to this? We had to create our own version of the React output target to make this work.

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

2 participants