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

Bug fix when a node get's removed after it's parent has already been removed #1303

Closed
wants to merge 1 commit into from

Conversation

MarkNijhof
Copy link

Fixed a but that happens when in certain cases when a state change decides a node and it's parent should be removed, because of timing issues (I think) it can happen that updatedChildren[j] is undefined when trying to remove itself from the parentNode.

I ran all tests and lint (which gave error's but not related to this pull request and tested the fix on our own code. I don't have a specific test case for this because I don't know how to trigger this scenario, but I assume this piece of code is already covered pretty well. If a specific test is required I am happy to do so with some guidance on how to trigger this behaviour without copying our own code :-)

A local fix is that instead of removing the child node I hide it instead, or applying this pull request.

…cides a node and it's parent should be removed, because of timing issues (I think) it can happen that updatedChildren[j] is undefined when trying to remove itself from the parentNode
@sophiebits
Copy link
Collaborator

I believe this should be fixed at a higher level; we're passing operations to DOMChildrenOperations in a way that isn't expected.

My #1157 fixes one of these causes. Can you try applying it to see if your problem is fixed? You might also be running into #1232 which I haven't had a chance to fix yet.

@sophiebits sophiebits closed this Mar 26, 2014
@MarkNijhof
Copy link
Author

I just pulled your queue-mc branch and did a grunt build. The error remains. I haven't been able to make a proper isolated repro of the issue yet. But I am pretty sure it has to do with altering child nodes from a React class while this class itself is being removed as well. And this decision is based upon the same data (displayError = false will both remove the class from the error list and the child node showing the error)

@sophiebits
Copy link
Collaborator

Well I'd be interested in seeing a repro if you can produce one. Ideally an isolated one but if you can't reduce it I'd be interested in looking at a live app showing the problem if you'd be able to send me the link. (You can see that the reduced repro case in #1147 was fairly complicated!)

@MarkNijhof
Copy link
Author

I'll try to create a proper repro in the next few days

@MarkNijhof
Copy link
Author

I added a test case and made a new pull request #1321 as I didn't know how to update this one with a new file. Please let me know if you need more :)

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.

2 participants