Skip to content

Conversation

@jamesrweb
Copy link
Member

@jamesrweb jamesrweb commented Mar 21, 2022

Fixes #142

Proposed Changes

  • Use refs to track the instance instead of state
  • Use types over interfaces for consistency

Additional notes

Please await a review from @jhrtn before merging.

Relates to the #145

@jamesrweb jamesrweb added the bug label Mar 21, 2022
@jamesrweb jamesrweb requested a review from yevdyko March 21, 2022 17:21
@jamesrweb jamesrweb self-assigned this Mar 21, 2022
@jamesrweb jamesrweb enabled auto-merge (squash) March 21, 2022 17:22
@yevdyko yevdyko force-pushed the update-instance-removal-logic-on-unmount branch from 5b447ef to 552c9e9 Compare March 21, 2022 17:56
Copy link
Contributor

@jhrtn jhrtn left a comment

Choose a reason for hiding this comment

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

Thanks for the prompt update! I've tested this in my minimal sandbox, the official example and the app I'm using the lib for and it appears to resolve the issue in all cases

Co-authored-by: Joseph Horton <[email protected]>
@yevdyko yevdyko force-pushed the update-instance-removal-logic-on-unmount branch from 836256f to f6c200e Compare March 21, 2022 21:08
Copy link
Contributor

@yevdyko yevdyko left a comment

Choose a reason for hiding this comment

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

@jamesrweb I've tested the fix using a minimal sandbox from @jhrtn and everything works as expected.

The only thing that can be improved is the unification of the mentioned variable name.

@jamesrweb jamesrweb requested a review from yevdyko March 22, 2022 22:46
Copy link
Contributor

@yevdyko yevdyko left a comment

Choose a reason for hiding this comment

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

Great job @jamesrweb! 🚢

@jamesrweb jamesrweb merged commit b34103c into master Mar 23, 2022
@jamesrweb jamesrweb deleted the update-instance-removal-logic-on-unmount branch March 23, 2022 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P5 instances not remove()'d on component unmount?

4 participants