Fix uncaught DOMException error #1752
Conversation
|
When can this happen? |
|
Hey @alexreardon! This happens when the dom is cycled out before pretty dnd can unmount (aka this settimeout get executed) |
|
When did you run into this? I'm guessing in a testing environment? |
|
Oh! Sorry didn't understand what you meant. I actually ran into this in my development and staging environment on the app I work on. We use Turbo links and sentry had caught this error when someone was navigating away from a react page that had react-beautiful-dnd on it. |
|
I am thinking we might want to add a test to ensure that we don't break people doing things like this |
|
For now I'm trying to understand what can cause the issue to occur |
|
Yup! thats the library. I can totally add a test. I can try to find a way to reproduce it. I was thinking that you could probably experience this if you used something like react-routter ( https://github.com/ReactTraining/react-router) potentially, but I think the easiest way to trigger this would be to unmount a root level react component at the same time as rendering a full new dom tree so that the timeout happens after the new page is rendered. I couldn't think of an easy way to reproduce it without spinning up a full rails app with turbo links and everything, but if we really want me to do that I totally can. In the mean time ill work on writing some tests. :) |
Let's add a test for that as I think it is the underlying technique |
|
Can you please add a test in |
|
(all the tests are moving over to there - all that folder uses react-testing-library) |
|
Yup! I can totally do that. |
| getBodyElement().removeChild(el); | ||
|
|
||
| // checking if element exists before remove to prevent errors | ||
| if (getBodyElement().contains(el)) { |
There was a problem hiding this comment.
I just checked - this is all good for ie11
https://developer.mozilla.org/en-US/docs/Web/API/Node/contains
|
Let me know when you would like me to take a look. Keep to get this in for v13! |
|
Getting on this this morning! should hopefully have tests up before lunch |
|
Hey @alexreardon it looks like there is a spec already in |
|
I pushed up the test and put it in the same file. It has react testing lib in there already I hope that is okay. Let me know if you want me to move it. |
|
And it now has passed all checks. Let me know if there is anything else you would like me to do! |
|
I am hoping to release 13.x soon and it would be good to get this in. Any chance you will have an opportunity to look at this soon? |
|
Thanks for verifying! I can't merge because its a protected branch and I am not authorized to do so. Never had this happen to me before so let me know what you need me to do on my side @alexreardon |
|
and I just realized that we are going to be in completely different time zones lol. Ill stay by my phone to merge this but let me know how I can merge into the branch that is protected. Ill google around to see if I can figure it out in the mean time. |
I can do the merging once the PR is ready to go |
Awesome! So maybe I missed it is there some feedback I need to address? |
|
It looks good, i'll merge it very soon |
This should fix #1751 :)