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

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

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

aweary
Copy link
Collaborator

@aweary aweary commented Aug 20, 2018

This reverts commit d60360c, reversing
changes made to bbf39e0.

@aweary aweary requested a review from ljharb August 20, 2018 17:28
@aweary
Copy link
Collaborator Author

aweary commented Aug 20, 2018

cc @gaearon

@aweary
Copy link
Collaborator Author

aweary commented Aug 20, 2018

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
Copy link
Contributor

gaearon commented Aug 20, 2018

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).

@@ -0,0 +1,104 @@
// Extracted from https://github.com/facebook/react/blob/7bdf93b17a35a5d8fcf0ceae0bf48ed5e6b16688/src/renderers/shared/fiber/ReactFiberTreeReflection.js#L104-L228
function findCurrentFiberUsingSlowPath(fiber) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

ljharb commented Aug 20, 2018

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
Copy link
Collaborator Author

aweary commented Aug 20, 2018

@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
Copy link
Member

ljharb commented Aug 20, 2018

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

@ljharb
Copy link
Member

ljharb commented Aug 20, 2018

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

@gaearon
Copy link
Contributor

gaearon commented Aug 20, 2018

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

@ljharb
Copy link
Member

ljharb commented Aug 20, 2018

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
Copy link
Member

ljharb commented Aug 20, 2018

(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
Copy link
Contributor

gaearon commented Aug 20, 2018

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
Copy link
Contributor

gaearon commented Aug 20, 2018

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
Copy link
Member

ljharb commented Aug 20, 2018

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
Copy link
Member

ljharb commented Aug 20, 2018

@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
Copy link
Contributor

gaearon commented Aug 20, 2018

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
Copy link
Member

ljharb commented Aug 20, 2018

got it, that makes sense.

@ljharb
Copy link
Member

ljharb commented Aug 20, 2018

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

…iler-react-16"

This reverts commit d60360c, reversing
changes made to bbf39e0.
@ljharb ljharb merged commit b7c054c into enzymejs:master Aug 24, 2018
ljharb added a commit that referenced this pull request Aug 25, 2018
 - [New] Add forwardRef support (#1592, @jquense)
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense)
 - [New] pass the adapter into `createMountWrapper` (#1592, @jquense)
 - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - update deps
 - [meta] ensure a license and readme is present in all packages when published
ljharb added a commit that referenced this pull request Aug 25, 2018
 - [New] Add forwardRef support (#1592, @jquense)
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary)
 - [Fix] `shallow`: prevent rerenders with PureComponents (#1786, @koba04)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - [Fix] `shallow`: `setState` after `setProps` calls `componentWillReceiveProps` (#1779, @peanutenthusiast)
 - [Fix] `mount`/`shallow`: be stricter on the wrapper’s setState/setProps callback
 - [Fix] `shallow`/`mount`: improve error message when wrapping invalid elements (#1759, @jgzuke)
 - update deps
 - [Refactor] remove most uses of lodash
 - [meta] ensure a license and readme is present in all packages when published
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.

3 participants