Skip to content

add enzyme-adapter-react-17 (no TODO tags) - #2534

Open
createthis wants to merge 18 commits into
enzymejs:masterfrom
createthis:feat/support-react17
Open

add enzyme-adapter-react-17 (no TODO tags)#2534
createthis wants to merge 18 commits into
enzymejs:masterfrom
createthis:feat/support-react17

Conversation

@createthis

@createthis createthis commented Aug 10, 2021

Copy link
Copy Markdown

Same as #2430 with TODO_17 tags removed.

How working on this PR made me feel:
derpy_dog

@codecov

codecov Bot commented Aug 10, 2021

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.06642% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.83%. Comparing base (8c55734) to head (46d0422).
⚠️ Report is 77 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 96.11% 16 Missing ⚠️
...ges/enzyme-adapter-react-17/src/detectFiberTags.js 85.24% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2534      +/-   ##
==========================================
- Coverage   96.31%   95.83%   -0.49%     
==========================================
  Files          49       53       +4     
  Lines        4207     4749     +542     
  Branches     1130     1304     +174     
==========================================
+ Hits         4052     4551     +499     
- Misses        155      198      +43     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread packages/enzyme-test-suite/test/shared/methods/setContext.jsx Outdated
expect(info).to.deep.equal(expectedInfo);
if (is('>= 17')) {
expect(info).to.have.property('componentStack');
expect(info.componentStack).to.match(/at Thrower (.+)\n/);

@createthis createthis Aug 10, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This change is necessary because the backtrace now includes local file paths in 17.x.

UNSAFE_componentWillReceiveProps() on setContext()
lifecycle methods disabled according to test.
instance.componentWillReceiveProps(props);
}
if (typeof instance.UNSAFE_componentWillReceiveProps === 'function') { // eslint-disable-line new-cap
instance.UNSAFE_componentWillReceiveProps(props); // eslint-disable-line new-cap

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This code corrects shallow()'s calling of componentWillReceiveProps() and UNSAFE_componentWillReceiveProps() under react 17.

it('works without memoizing', () => {
const wrapper = shallow(<RendersApp />);
expect(wrapper.debug()).to.equal('<App />');
expect(wrapper.debug()).to.equal(is('>= 17') ? '<AppMemoized />' : '<App />');

@createthis createthis Aug 13, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This appears to be due to a change in how React.memo() operates in React 17. Prior to 17 it seemed to return a copy of the component. In 17 it seems to return a reference.

const MemoForwardFoo = React.memo(React.forwardRef(Foo));
expect(adapter.displayNameOfNode(<MemoForwardFoo />)).to.equal('Memo(ForwardRef(CustomWrapper))');
if (is('>= 17')) {
expect(adapter.displayNameOfNode(<MemoForwardFoo />)).to.equal('Memo([object Object])');

@createthis createthis Aug 13, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm writing this a day after I took the screenshot, so it is possible it is the wrong screenshot, but I think the object looks like this:
Screen Shot 2021-08-11 at 4 03 27 PM

return isMemo(type) ? type.type : type;
}

function transformSuspense(renderedEl, prerenderEl, { suspenseFallback }) {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If I remove this method and all associated code, it goes from 5 suspense related test failures to 16 suspense related test failures. This code is clearly doing something useful.

expect(wrapper.debug()).to.equal(`<SuspenseComponent>
<Suspense fallback={{...}}>
<div />
${is('>= 17') ? '<Suspense mode="visible" />' : `<div />

@createthis createthis Aug 13, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Regrettably, I don't know why <Suspense mode="visible" /> is appearing instead of the expected html. The <Suspense /> subsystem in the 17 adapter is a bit beyond the limits of my domain knowledge so I am a little lost. I did my best to restore these suspense tests as completely as I could given my limitations. The <Suspense /> subsystem is listed as experimental by React, so I'm not sure how important it is: https://reactjs.org/docs/concurrent-mode-suspense.html

Comment thread CONTRIBUTING.md

In another terminal tab execute a specific test file for faster TDD test execution:
```bash
node_modules/.bin/mocha packages/enzyme-test-suite/build/ReactWrapper-spec.js

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Incorporated tips provided by https://github.com/ljharb back into documentation.

@createthis
createthis marked this pull request as ready for review August 13, 2021 19:00
@leopucci

This comment has been minimized.

@luiscosio

This comment has been minimized.

@EricKwan2014

This comment has been minimized.

@ljharb

This comment has been minimized.

@shiraze

shiraze commented Mar 31, 2022

Copy link
Copy Markdown

Hi guys, is this the correct place to ask what support my team and I can give to get a React 17 adaptor over the line? We've considered React-Testing-Library but the overhead of converting tests is so much that it might make better sense for us to provide support here.

@ljharb

ljharb commented Mar 31, 2022

Copy link
Copy Markdown
Member

@shiraze yes, if you can provide people and time, i'd be happy to set up a call to get everyone on the same page so we can get things landed.

@shiraze

shiraze commented Mar 31, 2022

Copy link
Copy Markdown

@ljharb excellent. Let's set up a call

@ljharb

ljharb commented Mar 31, 2022

Copy link
Copy Markdown
Member

@shiraze please reach out on twitter DMs or at gmail

@mak12

mak12 commented May 19, 2022

Copy link
Copy Markdown

Any update on this?

@shiraze

shiraze commented May 19, 2022

Copy link
Copy Markdown

Unfortunately I wasn't able to provide people and time

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

8 participants