Revert "Merge pull request #1399 from aweary/use-react-reconciler-rea…

[aweary]

This reverts commit d60360c, reversing
changes made to bbf39e0.

[aweary]

cc @gaearon

[aweary]

Looks like findCurrentFiberUsingSlowPath has been updated since we made this change, so I'll have to investigate integrating those updates for all supported versions that use this.

[gaearon]

Thanks! Do you mind verifying algorithm against the current one? I don’t think it changed but worth checking. It’s okay if invariants are missing (better than depending on constants).

[ljharb]

I don't understand why this is an improvement - we were locked to v0.7.0 before, and now this is just inlining it. Why can't this be pulled from react-reconciler, or why can't react-reconciler be updated?

[ljharb]

I'm all for making changes to make things easier for the react team; but copy-pasting code doesn't seem like a good approach for anyone.

[aweary]

@gaearon it looks like the algorithm hasn't changed, with the exception (heh) of the invariants. See the diff below.

@ljharb I'll let @gaearon answer that, I'm not totally familiar with what problems this is causing for us at Facebook.

findCurrentFiberUsingSlowPath Diff

---

1,2c1,2
< function findCurrentFiberUsingSlowPath(fiber) {
<   const { alternate } = fiber;
---
> export function findCurrentFiberUsingSlowPath(fiber: Fiber): Fiber | null {
>   let alternate = fiber.alternate;
3a4,12
>     // If there is no alternate, then we only need to check if it is mounted.
>     const state = isFiberMountedImpl(fiber);
>     invariant(
>       state !== UNMOUNTED,
>       'Unable to find node on an unmounted component.',
>     );
>     if (state === MOUNTING) {
>       return null;
>     }
11,13c20,22
<   while (true) { // eslint-disable-line
<     const parentA = a.return;
<     const parentB = parentA ? parentA.alternate : null;
---
>   while (true) {
>     let parentA = a.return;
>     let parentB = parentA ? parentA.alternate : null;
18c27,28
<      // If both copies of the parent fiber point to the same child, we can
---
> 
>     // If both copies of the parent fiber point to the same child, we can
22c32
<       let { child } = parentA;
---
>       let child = parentA.child;
25a36
>           assertIsMounted(parentA);
29a41
>           assertIsMounted(parentA);
36c48
<       throw new Error('Unable to find node on an unmounted component.');
---
>       invariant(false, 'Unable to find node on an unmounted component.');
38c50,51
<      if (a.return !== b.return) {
---
> 
>     if (a.return !== b.return) {
52c65
<       let { child } = parentA;
---
>       let child = parentA.child;
70c83
<         ({ child } = parentB);
---
>         child = parentB.child;
86,89c99,103
<         if (!didFindChild) {
<           throw new Error('Child was not found in either parent set. This indicates a bug ' +
<             'in React related to the return pointer. Please file an issue.');
<         }
---
>         invariant(
>           didFindChild,
>           'Child was not found in either parent set. This indicates a bug ' +
>             'in React related to the return pointer. Please file an issue.',
>         );
91a106,111
> 
>     invariant(
>       a.alternate === b,
>       "Return fibers should always be each others' alternates. " +
>         'This error is likely caused by a bug in React. Please file an issue.',
>     );
92a113,118
>   // If the root is not a host container, we're in a disconnected tree. I.e.
>   // unmounted.
>   invariant(
>     a.tag === HostRoot,
>     'Unable to find node on an unmounted component.',
>   );

---

[ljharb]

I think that we'd need more than a vague "it's causing problems" to justify inlining any dependency.

[ljharb]

I also see that react-reconciler is at v0.12.0 - could we update to that?

[gaearon]

Different adapters (even within 16) would need to depend on different versions.

[ljharb]

That's fine, we already have 16.1, 16.2, and 16.3 adapters, and soon i'll do a breaking change on the 16 adapter to require ^16.4.

[ljharb]

(it'd be great if those requirements were documented in the range for react in peerDependencies in react-reconciler; v0.12.0 claims "^16.0.0")

[gaearon]

Hmm. I see. If you don’t mind bumping majors when there are incompatible internal changes then you could make it work.

That said, we still don’t provide guarantees about this package working as intended here. The fact that constants match between react-dom and react-reconciler is somewhat accidental. This is a bit like using a utility from React Native and hoping it would work on React DOM.

[gaearon]

it'd be great if those requirements were documented in the range for react in peerDependencies in react-reconciler; v0.12.0 claims "^16.0.0"

I think I already tried to explain this in
#1399 (comment). There’s no relationship meant between them at all. The fact that this works is an accident. They happen to be released at the same time as an artefact of release cycle. That may change. There was no intentional compatibility between these packages. It may break in the future when our build optimisations become more aggressive.

[ljharb]

I understand that; but I'd still expect the peerDep range to match what it's already compatible with, even if it won't be compatible with newer things - ie, 16.0.x - 16.4.x, or =16.4.1, etc. It doesn't have to provide future guarantees to provide correct current info.

[ljharb]

@gaearon i don't actually care about using react-reconciler specifically, ftr - i just don't want to have to inline code unnecessarily. It'd be great if react exposed a "toTree" implementation of some kind though - then we'd be able to rely on that for any given future react version.

[gaearon]

The react-reconciler package is compatible with any 16.x version of the react package. That’s what the peer dependency expresses.

There are no guarantees around compatibility between react-dom and react-reconciler packages. Just like there isn’t any between React DOM and React Native. This is why they don’t have peer dependencies on each other.

[ljharb]

got it, that makes sense.

[ljharb]

So whatever problem this PR is attempting to fix - could we add a test that would fail without this PR?

[ljharb]

As discussed; this will be merged as part of #1790.