[Fiber] String refs and owner tracking by acdlite · Pull Request #8099 · facebook/react

acdlite · 2016-10-26T00:04:43Z

Implements string refs using callback closures.

Still need to update existing ref tests to work with Fiber.

sebmarkbage · 2016-10-26T00:17:09Z

src/renderers/shared/fiber/ReactChildFiber.js

+      const inst = ownerFiber.stateNode;
+      const ref = function(value) {
+        const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs;
+        refs[element.ref] = value;


We should store the ref string in a separate variable so that we don't capture the entire element and all its props in the closure.

sebmarkbage · 2016-10-26T00:18:15Z

src/renderers/shared/fiber/ReactChildFiber.js

+function transferRef(fiber: Fiber, element: ReactElement<any>) {
+  if (typeof element.ref === 'string' && element._owner) {
+    // Should we check if this string refs is equal to the previous string ref
+    // and reuse it?


I'm not sure if it is faster or not actually. But we could try attaching string/owner as properties on the function. We only pay for it if you use it.

sebmarkbage · 2016-10-26T00:22:04Z

src/renderers/shared/fiber/ReactFiberBeginWork.js


-    var nextChildren = fn(props);
+    if (__DEV__) {
+      ReactCurrentOwner.current = workInProgress;


I thought we decided that we didn't need this for functional components? EDIT: Oh, it is DEV only. nvm.

I thought what we decided was we didn't need it when constructing a class component. Still need it for rendering a functional component. Might be best to remove this until it's actually needed regardless.

sebmarkbage · 2016-10-26T00:24:50Z

src/renderers/shared/fiber/ReactChildFiber.js

+  if (typeof element.ref === 'string' && element._owner) {
+    // Should we check if this string refs is equal to the previous string ref
+    // and reuse it?
+    const ownerFiber : ?Fiber = (element._owner : any);


This is not guaranteed to be a Fiber and it is not guaranteed to be a ClassComponent since you attach owners to functional components too.

Oh yeah I even imported ClassComponent to do the comparison but then forgot :D

sebmarkbage · 2016-10-26T00:25:22Z

src/renderers/shared/fiber/ReactChildFiber.js

 } = ReactTypeOfSideEffect;

+function transferRef(fiber: Fiber, element: ReactElement<any>) {
+  if (typeof element.ref === 'string' && element._owner) {


If this is a string and there is no owner, we should probably not set the fiber.ref to a string. Maybe move the second condition inside.

sebmarkbage · 2016-10-26T00:28:08Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

    const instance = workInProgress.stateNode;
+    ReactCurrentOwner.current = workInProgress;
    const nextChildren = instance.render();
+    ReactCurrentOwner.current = null;


This is doesn't reset it if the render throws. We should probably just move ReactCurrentOwner.current = null; out to @gaearon's top level try-block in #8095. So that we just reset it when we're completely done reconciling.

Cool I'll wait until that one gets merged

Or you can just add it at the end of performWork without the try/finally so that we can land this and get some test scores up! :)

That also works :)

sebmarkbage · 2016-10-26T00:28:44Z

Alright. Accepted, pending comments.

aweary · 2016-10-26T02:15:56Z

src/renderers/shared/fiber/ReactChildFiber.js

 const isArray = Array.isArray;

 const {
+  ClassComponent,


Looks like ClassComponent isn't used anywhere?

Yeah I imported it for this purpose but forgot: https://github.com/facebook/react/pull/8099/files#r85031873

:D

acdlite · 2016-10-26T03:18:55Z

653 tests failed, 3 tests skipped, 838 tests passed

sebmarkbage · 2016-10-26T05:09:57Z

src/isomorphic/hooks/ReactComponentTreeHook.js

+    if (currentOwner && typeof currentOwner._debugID === 'number') {
+      var id = currentOwner && currentOwner._debugID;
+      info += ReactComponentTreeHook.getStackAddendumByID(id);
+    }


I'm actually about to try to make this file work with Fiber but this is fine too.

Yeah tbh I barely looked at what this file does; just trying to get it merged for now :D

Still one remaining type error that I'm not sure how to fix

sebmarkbage · 2016-10-26T06:53:31Z

Looks like you still have one more Flow error.

acdlite · 2016-10-26T06:55:37Z

@sebmarkbage Yeah can't figure it out yet :(

gaearon · 2016-10-26T11:58:48Z

src/renderers/shared/fiber/ReactChildFiber.js

+        const ref = function(value) {
+          const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs;
+          refs[stringRef] = value;
+        }


Missing semicolon.

gaearon · 2016-10-26T11:59:18Z

src/renderers/shared/fiber/ReactChildFiber.js

+          return;
+        }
+        const inst = ownerFiber.stateNode;
+        const ref = function(value) {


This function won't get a name. We should make any functions calling user code named for better stack traces.

Never mind, I misunderstood. This doesn't call into user code.

gaearon · 2016-10-26T12:43:08Z

The Flow issue is because ReactCurrentOwner.current can now be a Fiber, so there's no guarantee _warnedAboutRefsInRender exists on it.

gaearon · 2016-10-26T13:37:53Z

I fixed it up so we can get this merged and avoid conflicts later.

acdlite · 2016-10-26T13:50:07Z

Ha that's one way to fix it! Thanks for merging

gaearon · 2016-10-26T13:56:07Z

Yea since it's RN I don't think it's an issue, we won't get there very soon anyway.

gaearon · 2016-10-26T13:56:46Z

We could also add $$typeof to internal instances and Fibers for nominal typing.

* Implement string refs using callback closures * Merge Fiber type to avoid Flow intersection bugs Still one remaining type error that I'm not sure how to fix * Fix Flow issue with an unsafe cast * Fix missing semicolon * Add a type import I missed earlier

acdlite changed the title ~~[Fiber ] String refs and owner tracking~~ [Fiber] String refs and owner tracking Oct 26, 2016

facebook-github-bot added the GH Review: review-needed label Oct 26, 2016

sebmarkbage mentioned this pull request Oct 26, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

facebook-github-bot added the CLA Signed label Oct 26, 2016

sebmarkbage reviewed Oct 26, 2016

View reviewed changes

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Oct 26, 2016

aweary reviewed Oct 26, 2016

View reviewed changes

Implement string refs using callback closures

aa14cc2

acdlite force-pushed the fiberstringrefs branch from e221ec0 to aa14cc2 Compare October 26, 2016 03:17

sebmarkbage reviewed Oct 26, 2016

View reviewed changes

Merge Fiber type to avoid Flow intersection bugs

338683f

Still one remaining type error that I'm not sure how to fix

acdlite force-pushed the fiberstringrefs branch from 6c8c3f3 to 338683f Compare October 26, 2016 05:15

gaearon reviewed Oct 26, 2016

View reviewed changes

gaearon added 3 commits October 26, 2016 14:08

Fix Flow issue with an unsafe cast

d3fb83c

Fix missing semicolon

376b8f3

Add a type import I missed earlier

68c0915

gaearon merged commit d5eff3b into master Oct 26, 2016

gaearon deleted the fiberstringrefs branch October 26, 2016 13:37

Conversation

acdlite commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acdlite Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acdlite commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented Oct 26, 2016

Uh oh!

acdlite commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Oct 26, 2016

Uh oh!

gaearon commented Oct 26, 2016

Uh oh!

acdlite commented Oct 26, 2016

Uh oh!

gaearon commented Oct 26, 2016

Uh oh!

gaearon commented Oct 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sebmarkbage Oct 26, 2016 •

edited

Loading

acdlite Oct 26, 2016 •

edited

Loading