Skip to content

Resolve unmounting instance issue #145

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 16, 2022
Merged

Resolve unmounting instance issue #145

merged 1 commit into from
Feb 16, 2022

Conversation

jamesrweb
Copy link
Member

Fixes issue #142

Proposed Changes

  • Adds a new effect handler to remove the instance when the component is unmounted
  • Update the dependencies to their latest versions
  • Update the example to test mount and unmount behaviours
  • Added a test to check that when the component is unmounted, the containing element has no innerHTML content. This in turn triggers the unmount effect.

@jamesrweb jamesrweb added bug enhancement dependencies Pull requests that update a dependency file security This label applies to security issues labels Feb 13, 2022
@jamesrweb jamesrweb requested a review from yevdyko February 13, 2022 21:06
@jamesrweb jamesrweb self-assigned this Feb 13, 2022
@jamesrweb jamesrweb linked an issue Feb 13, 2022 that may be closed by this pull request
P5 instances not remove()'d on component unmount? #142
Closed
@jamesrweb jamesrweb enabled auto-merge (squash) February 13, 2022 21:06
@jamesrweb jamesrweb mentioned this pull request Feb 13, 2022
Closed
fredrikelmegren
fredrikelmegren approved these changes Feb 16, 2022
View reviewed changes
src/index.tsx
}, [props]);
}, [props, instance]);

useIsomorphicEffect(() => () => instance?.remove(), []);
Copy link

Choose a reason for hiding this comment

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

Haven't had time to test it out, but this here line looks like it'd indeed solve #142 😃

yevdyko
yevdyko approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@yevdyko yevdyko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the extra testing @jamesrweb!

Looks good to me 🐑

@jamesrweb jamesrweb merged commit d4c5ce0 into master Feb 16, 2022
@jamesrweb jamesrweb deleted the resolve-unmounting-instance-issue branch February 16, 2022 23:07
@yevdyko yevdyko mentioned this pull request Mar 22, 2022
Merged
jamesrweb added a commit that referenced this pull request Aug 15, 2022
@jamesrweb
Resolve unmounting instance issue (#145)
c6d8df3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredrikelmegren fredrikelmegren fredrikelmegren approved these changes

@yevdyko yevdyko yevdyko approved these changes

Assignees

@jamesrweb jamesrweb

Labels
bug dependencies Pull requests that update a dependency file enhancement security This label applies to security issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P5 instances not remove()'d on component unmount?
3 participants
@jamesrweb @fredrikelmegren @yevdyko