Skip to content

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

Closed
WickyNilliams wants to merge 3 commits intostenciljs:mainfrom
WickyNilliams:bugfix/cleanup-events-react-output
Closed

fix(react): cleanup events if they are no longer passed as props#148
WickyNilliams wants to merge 3 commits intostenciljs:mainfrom
WickyNilliams:bugfix/cleanup-events-react-output

Conversation

@WickyNilliams
Copy link
Copy Markdown
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
Copy Markdown

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

@arielsalminen
Copy link
Copy Markdown

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

@christian-bromann
Copy link
Copy Markdown
Member

This patch is not relevant anymore as we have moved to @lit/react for wrapping Stencil components within React. Thanks for the contribution.

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.

3 participants