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: 'ref' not available reliably #200

Merged
merged 9 commits into from
Jan 18, 2019

Conversation

bryphe
Copy link
Member

@bryphe bryphe commented Jan 8, 2019

Issue: Discovered this while working on a prototype slider control: bryphe/prototype/slider.

Snippet of code with the issue:

let (ref, setRef) = useState(None);
         ...
let onMouseDown = (_evt) => {
       switch(ref) {
       | Some(r) => print_endline("got ref: " ++ string_of_int(r#getInternalId()));
       | None => print_endline("no ref");
       };
}
         ...
<view onMouseDown style ref={(r) => setRef(r)}> 

Defect: The ref would not be available reliably. The ref callback is being called correctly, but when we use setRef it's happening at a time when the instance hasn't been mounted completely in the reconciler - so it's not able to persist the state. Later renders give back None.

Fix: My proposed fix for this is to have this event be 'queued' such that we only dispatch it when reconciliation is complete. This is doable since we have the ref model implemented completely in 'user-space' - in other words, reason-reactify doesn't model ref today.

This would create an event queue for nodes - and only dispatch it on request (which revery will flush these pending events after every render).

Open Questions

  • Should we look at add ref as a primitive by default in reason-reactify. If so, would it make sense to have a useRef hook we include as well? If we push it to the framework - we might be able to come up with a more efficient implementation.
  • This also seems related to the idea of batching updates used in ReactMini and mentioned in Hooks with slots reason-reactify#51 - which also makes me consider that the reconciler would be the right place to push-back this fix. Depending on how that issue is fixed - I suspect the batching updates - enforcing that 'useState' is called at a correct time with more precision - would also address the issue.

TODO:

  • Add test case exercising the issue
  • Implement event-queue fix
  • Better comments in the Node module
  • Fix formatting

@bryphe bryphe changed the title [WIP] Bugfix: 'ref' not available reliably Bugfix: 'ref' not available reliably Jan 8, 2019
@bryphe
Copy link
Member Author

bryphe commented Jan 18, 2019

Fix with the upgrade to brisk-reconciler - no need to queue up ref callbacks anymore.

@bryphe bryphe merged commit bfaac51 into master Jan 18, 2019
@bryphe bryphe deleted the bryphe/bugfix/ref-not-available-after-reconcile branch January 18, 2019 18:45
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.

1 participant