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

Bugfix: conditional check around ellipsis removal to prevent errors in unexpected situations #129

Merged

Conversation

kevinparkerson
Copy link
Contributor

Fixes #128

Adds a conditional check around ellipsis removal in ComponentWillUnmount to help pass test suites and other situations where it might be missing

…unt to help pass test suites and other situations where it might be missing
@kevinparkerson
Copy link
Contributor Author

@pablosichert thank you so much for this component! It is extremely useful and I've found it more reliable than other options 👍 I'm sure you're quite busy but hopefully you'll have some time to merge this simple PR so it is integrates with testing suites without mocks

@pablosichert
Copy link
Owner

Hey there! Sounds fair enough to include that check.

I'm still curious when that happens, could you elaborate?

@kevinparkerson
Copy link
Contributor Author

kevinparkerson commented Apr 14, 2020

@pablosichert Sure thing! It was occurring when running Jest / Storybook tests, likely due to its use of JSDOM instead of a real browser environment. There's probably a way to create a mock or workaround for this issue, but having this simple check makes it easier to integrate for everyone using it without those added steps

Additionally I've seen the same Cannot read property 'parentNode' of undefined error emitted by this source in the console from time to time when visiting a different page in a Next.js app.

@pablosichert
Copy link
Owner

If it only happened for the tests, I would have considered to detect that at instantiation.

But with the risk of it happening at runtime, applying the check there seems like the better option.

Thank you for the PR!

@pablosichert pablosichert merged commit cce8d0e into pablosichert:master Apr 14, 2020
@kevinparkerson
Copy link
Contributor Author

@pablosichert No problem! Thank you for the quick review and merge!

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.

ComponentWillUnmount ellipsis removal fails in test suites and other situations
2 participants