Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Change refs in one pass, making switching refs atomic #98

Merged
merged 2 commits into from
May 22, 2018
Merged

Conversation

LPGhatguy
Copy link
Contributor

@LPGhatguy LPGhatguy commented May 21, 2018

This is a minor change that ideally shouldn't be user-visible.

Before #92, there was a bug in Roact where when refs were updated, nothing would happen. There was a typo in the reconciler code that resulted in the comparison in the reconciler always determining that refs never changed!

After #92, an issue was unveiled by @ZoteTheMighty via some internal Roact code that means that with the typical use of a ref:

[Roact.Ref] = (rbx)
   self.foo = rbx
end

...there is a period of time where self.foo will be nil when Roact nullifies the old ref, makes changes, and then passes the same object to the new ref, since by function identity it changed. This doesn't affect code that keeps a stable ref function, and is only visible to code that acts on a ref in response to a change event.

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Tests added and updated I don't think we can test this

@coveralls
Copy link

coveralls commented May 21, 2018

Coverage Status

Coverage decreased (-0.03%) to 90.382% when pulling 2fd8db4 on atomic-refs into f8edb9f on master.

@LPGhatguy
Copy link
Contributor Author

Coverage drop is in... PureComponent?

@ZoteTheMighty
Copy link
Contributor

Coveralls seems to have gotten very confused about what actually changed...

@ZoteTheMighty
Copy link
Contributor

Can confirm that the edge case we saw is resolved by this. Looks good so far, I'll take a look and approve once there's a test that looks good.

@LPGhatguy LPGhatguy merged commit 01f327b into master May 22, 2018
@LPGhatguy LPGhatguy deleted the atomic-refs branch May 22, 2018 20:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants