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

Fix an edge case in ReactFiberUpdateQueue #8975

Closed
wants to merge 1 commit into from
Closed

Fix an edge case in ReactFiberUpdateQueue #8975

wants to merge 1 commit into from

Conversation

jddxf
Copy link
Contributor

@jddxf jddxf commented Feb 10, 2017

Correct me if I misunderstood something.

@jddxf jddxf changed the title alternate update queue's last should be set to update instead of null when the insertion positions are the same alternate update queue's last should be set to update instead of null Feb 10, 2017
@acdlite
Copy link
Collaborator

acdlite commented Feb 10, 2017

Good catch! Hmm, typically we'd want to make sure we have a unit test to prevent a regression, but with edge cases like this that are so implementation specific it may not be worth it. Do you mind giving it a shot?

@acdlite
Copy link
Collaborator

acdlite commented Feb 10, 2017

Actually don't sweat it. The path you'd have to take to trigger this edge case is pretty complex. I'll give it a shot before I merge.

@jddxf
Copy link
Contributor Author

jddxf commented Feb 10, 2017

That's fine.Thanks!

@gaearon
Copy link
Collaborator

gaearon commented Feb 11, 2017

That’s pretty cool, thank you for finding it. Did you just walk into it reading the source?

@jddxf jddxf changed the title alternate update queue's last should be set to update instead of null Fix an edge case in ReactFiberUpdateQueue Feb 11, 2017
@jddxf
Copy link
Contributor Author

jddxf commented Feb 11, 2017

@gaearon Yes.I am trying to understand the fiber :)

@gaearon
Copy link
Collaborator

gaearon commented Feb 11, 2017

So are we. 😄

@jddxf
Copy link
Contributor Author

jddxf commented Feb 11, 2017

lol.

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2017

@acdlite Is this still relevant?

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

I'm going to close since this is outdated. If the bug still exists please re-submit with instructions to reproduce the problem. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants