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

[New] add enzyme-adapter-react-17 #2430

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

layershifter
Copy link

Fixes #2429.

}
}
ReactDOM.render(React.createElement(Tester), container);
return inst._reactInternals.child;
Copy link
Author

Choose a reason for hiding this comment

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

This is the main change there, facebook/react#18377

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Attention: Patch coverage is 91.60156% with 43 lines in your changes missing coverage. Please review.

Project coverage is 95.61%. Comparing base (cdfb1c6) to head (898978f).
Report is 88 commits behind head on master.

Files with missing lines Patch % Lines
...pter-react-17/src/findCurrentFiberUsingSlowPath.js 68.42% 18 Missing ⚠️
...zyme-adapter-react-17/src/ReactSeventeenAdapter.js 95.90% 16 Missing ⚠️
...ges/enzyme-adapter-react-17/src/detectFiberTags.js 85.24% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2430      +/-   ##
==========================================
- Coverage   96.12%   95.61%   -0.52%     
==========================================
  Files          49       53       +4     
  Lines        4004     4516     +512     
  Branches     1123     1288     +165     
==========================================
+ Hits         3849     4318     +469     
- Misses        155      198      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

There's a reason that issue did not have the "help wanted" label on it; thanks for the contribution, but I already have the majority of this change coded and am in the process of testing it.

Please leave this PR open so i can update it, otherwise the PR ref will be left orphaned.

@silver-snoopy
Copy link

Dear @layershifter thanks for the PR!

Any update on this?
React team has released V17 2 days ago, and It would be very much appreciated if this PR could go through

@wojtekmaj
Copy link

FYI, this might be affected by the bug reported in my temporary adapter repo:

wojtekmaj/enzyme-adapter-react-17#1

@layershifter
Copy link
Author

Dear @layershifter thanks for the PR!

Any update on this?
React team has released V17 2 days ago, and It would be very much appreciated if this PR could go through

I stopped to work on it after @ljharb comment: #2430 (review). I am also interested in getting this.

@ljharb
Copy link
Member

ljharb commented Oct 23, 2020

Indeed, that linked bug is one of the blockers I've run into in my local work.

I'd be more than happy to pull in additional commits to this PR, and to my local implementation, in service of getting something published ASAP.

@ljharb
Copy link
Member

ljharb commented Oct 26, 2020

I've rebased this branch and included my WIP. There's still 8 failing tests (and 2 I've temporarily patched), that fall into these categories:

  1. unknown node with tag 23. tag 23 is "OffscreenComponent" in the latest react-reconciler repo, but I have no idea how to render something useful here.
  2. simulate issues in shallow: marked with "TODO: figure out why this is broken in shallow rendering in react 17"
  3. issues because the getComponentStack function in enzyme-adapter-utils no longer creates an accurate error stack for React 17.

Please post links to commits/branches, and I'll pull them into the PR.

@eps1lon
Copy link
Contributor

eps1lon commented Oct 26, 2020

  1. unknown node with tag 23. tag 23 is "OffscreenComponent" in the latest react-reconciler repo, but I have no idea how to render something useful here.

getLazyFiber("div").return.return.tag should do: https://codesandbox.io/s/offscreencomponent-tag-9hsq9

@ljharb
Copy link
Member

ljharb commented Oct 26, 2020

@eps1lon awesome, thanks! the other side of this is, how do i locate, off of the FiberNode, what should be rendered? It's got a bunch of circular references. The best I could come up with makes this:

class Component extends React.Component {
        render() {
          return (
            <div>test</div>
          );
        }
      }

      const SuspenseComponent = () => (
        <Suspense fallback={Fallback}>
          <Component />
        </Suspense>
      );

map to this:

<SuspenseComponent>
  <Suspense fallback={[Function: Fallback]}>
    <Suspense mode="visible" />
  </Suspense>
</SuspenseComponent>

with this:

     case FiberTags.OffscreenComponent: {                                                                                                                                                                   
       console.log(node.return.memoizedProps.children);                                                                                                                                                     
       return {                                                                                                                                                                                             
         nodeType: 'function',                                                                                                                                                                              
         type: Suspense,                                                                                                                                                                                    
         props: { ...node.memoizedProps },                                                                                                                                                                  
         key: ensureKeyOrUndefined(node.key),                                                                                                                                                               
         ref: node.ref,                                                                                                                                                                                     
         instance: null,                                                                                                                                                                                    
         rendered: childrenToTree(nodeToHostNode(node.return.memoizedProps.children)),                                                                                                                      
       };                                                                                                                                                                                                   
     }                                                                                     

@eps1lon
Copy link
Contributor

eps1lon commented Oct 26, 2020

I don't have more context on how the OffscreenComponent is handled internally in React. From what I could gather it's used in different scenarios. Some only available with experimental APIs. If you can add a test I might be able to help.

@ljharb
Copy link
Member

ljharb commented Oct 26, 2020

@eps1lon thanks! the test is already there; i've updated the branch to have my best output so far.

@eps1lon
Copy link
Contributor

eps1lon commented Oct 29, 2020

@ljharb
With

diff --git a/packages/enzyme-adapter-react-17/src/ReactSeventeenAdapter.js b/packages/enzyme-adapter-react-17/src/ReactSeventeenAdapter.js
index 0e69131..bb3477d 100644
--- a/packages/enzyme-adapter-react-17/src/ReactSeventeenAdapter.js
+++ b/packages/enzyme-adapter-react-17/src/ReactSeventeenAdapter.js
@@ -299,16 +299,7 @@ function toTree(vnode) {
     case FiberTags.Lazy:
       return childrenToTree(node.child);
     case FiberTags.OffscreenComponent: {
-      console.log(node.return.memoizedProps.children);
-      return {
-        nodeType: 'function',
-        type: Suspense,
-        props: { ...node.memoizedProps },
-        key: ensureKeyOrUndefined(node.key),
-        ref: node.ref,
-        instance: null,
-        rendered: childrenToTree(nodeToHostNode(node.return.memoizedProps.children)),
-      };
+      return toTree(node.child);
     }
     default:
       throw new Error(`Enzyme Internal Error: unknown node with tag ${node.tag}`);
diff --git a/packages/enzyme-test-suite/test/ReactWrapper-spec.jsx b/packages/enzyme-test-suite/test/ReactWrapper-spec.jsx
index c77bf15..234da0b 100644
--- a/packages/enzyme-test-suite/test/ReactWrapper-spec.jsx
+++ b/packages/enzyme-test-suite/test/ReactWrapper-spec.jsx
@@ -1112,7 +1112,7 @@ describeWithDOM('mount', () => {
       }
     }
 
-    it.only('finds Suspense and its children when no lazy component', () => {
+    it('finds Suspense and its children when no lazy component', () => {
       class Component extends React.Component {
         render() {
           return (
@@ -1130,7 +1130,6 @@ describeWithDOM('mount', () => {
       const wrapper = mount(<SuspenseComponent />);
 
       expect(wrapper.is(SuspenseComponent)).to.equal(true);
-      console.log(wrapper.debug());
       expect(wrapper.find(Component)).to.have.lengthOf(1);
       expect(wrapper.find(Fallback)).to.have.lengthOf(0);
     });

the suspense tests are passing. npm run test:only is still failing on 2 component stack related tests though.

@ljrodriguez1

This comment has been minimized.

@gurufordy
Copy link

Hi all, firstly thanks for all your hard work on this project.

Has this PR stalled? I'm not quite sure what state it's in apart from it's failing code coverage right now.

I'm probably one of many people who rely on packages that use this under the hood and are not able to upgrade to React 17 (safely) until this gets updated.

https://xkcd.com/2347/

@jonnylynchy

This comment was marked as spam.

@hodgef

This comment was marked as off-topic.

@mak12

This comment was marked as spam.

@anton-johansson

This comment was marked as off-topic.

@igorpupkinable
Copy link

igorpupkinable commented Jul 15, 2022

What are the options for developers and projects with thousands of Enzyme for React 16 tests? Is this worth any more waiting or better to migrate off Enzyme?

@ljharb
Copy link
Member

ljharb commented Jul 15, 2022

@igorpupkinable unfortunately there isn't any alternative to enzyme for the majority of what enzyme provides.

I still intend to complete this task; I'm hoping next month.

@chyzwar

This comment was marked as spam.

@csvan
Copy link

csvan commented Jul 21, 2022

@chyzwar @igorpupkinable RTL and Enzyme are VERY different, and people who migrate to RTL expecting to get feature parity with Enzyme are going to be sorely disappointed.

@createthis
Copy link

RTL and Enzyme are VERY different, and people who migrate to RTL expecting to get feature parity with Enzyme are going to be sorely disappointed.

This decision by the react team and airbnb team remind me a bit of the angular 2 -> 3 decision making process. It makes sense from a today and future development perspective, but it gives zero thought to existing infrastructure with thousands of established tests. It feels like that's your problem not mine. These kinds of decisions historically have not gone over super well with the community. Maybe it doesn't matter though. Time will tell.

@ljharb
Copy link
Member

ljharb commented Jul 21, 2022

@createthis which decision are you referring to?

React decided they no longer wanted to have to consider enzyme users when changing React internals.

Airbnb decided they didn't want to maintain the package, and I've been doing so, unfunded, on my own, for many years.

Nobody has decided enzyme shouldn't or won't support React 17+, it's just that out of all the people who've complained about their company's codebases, ZERO of them have provided funding or programmer time to make it happen.

@createthis
Copy link

@createthis which decision are you referring to?

React decided they no longer wanted to have to consider enzyme users when changing React internals.

Airbnb decided they didn't want to maintain the package, and I've been doing so, unfunded, on my own, for many years.

Yes.

@ljharb
Copy link
Member

ljharb commented Jul 21, 2022

To be clear though, while I certainly am not happy about those two companies' decisions here, the blame (if there must be "blame") is shared equally by every single user of the project (individuals and/or corporations) who isn't contributing time and/or money to supporting it.

Open Source is not an inexhaustible source of free labor, as much as I wish the world's legal and economic systems were set up for that to be feasible.

@createthis
Copy link

Agreed. It's all just js in the end. Anyone can contribute. I did my best to move the ball forward. I got stuck and blocked on the rebase issues. If anyone could unblock me I might be willing to try again.

That said, without major company support from fb or airbnb or another large company using the ecosystem, it may be an uphill battle not worth fighting. Sometimes it's better to just go with the flow.

@hodgef
Copy link

hodgef commented Jul 22, 2022

To be clear though, while I certainly am not happy about those two companies' decisions here, the blame (if there must be "blame") is shared equally by every single user of the project (individuals and/or corporations) who isn't contributing time and/or money to supporting it.

Open Source is not an inexhaustible source of free labor, as much as I wish the world's legal and economic systems were set up for that to be feasible.

While I sympathize with this statement, I would suggest that a maintainer should still ensure that critical issues are resolved in a timely fashion (such as people not being able to update React because this dep is holding them back). If this cannot be done in say 2 years, then it's normal for users to be considering alternatives.

I know what being a lone maintainer feels like (maybe not at this scale though!), but I think it wouldn't be helpful for me to blame users for my inability to maintain the project that I own. I'm the maintainer, after all. If there was a situation where I couldn't maintain my projects, I would put a disclaimer or archive the project for the reasons mentioned (lack of time, lack of funding, lack of help, etc). Until a firm commitment to fix/prioritization is made by the maintainer there will inevitably be a cloud of doubt and people will be asking for status and talking about alternatives, it's just normal.

Another thing I want to add: It's not because a project is OSS that people will help maintain the project. People use OSS mostly to ensure deps are peer-reviewed and do not contain malware. People will still ping the maintainer, because only the maintainer is expected to maintain the project.

Hopefully the React team/@gaearon and Enzyme can reach some solution (maybe a compatibility layer) as this project is undeniably useful to many.

@eps1lon eps1lon mentioned this pull request Aug 2, 2022
4 tasks
@ljharb ljharb force-pushed the master branch 3 times, most recently from 43eb75e to 39e6b1f Compare November 3, 2022 21:47
@ljharb ljharb reopened this Oct 21, 2023
@ljharb ljharb reopened this Mar 15, 2024
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.

Support for React 17