Support wrappingComponent & wrappingComponentProps #157

kevinweber · 2022-02-17T16:35:46Z

Support for wrappingComponent has been requested in #99 and this PR adds it for components rendered using the shallow and mount Enzyme utils.

How was this tested?

I added tests to the project and all are passing.
I verified that the changes in this PR solve the issue for my organization, allowing us to use wrappingComponent and wrappingComponentProps for both shallow and mount components.

Note for my future self: this is how I verified the changes locally…

Make sure yalc is installed (as recommended in the contributing guide): yarn global add yalc
Run yalc publish in the (forked) project locally.
Run this in the other project where the dependency should be tested: yalc add enzyme-adapter-preact-pure. This updates the package.json and lock file. Then run yarn install.
After making a change, run yalc publish --push and the other project will use the latest code

Tips for faster development:

Auto-run tests on file change: nodemon --watch "test/**" --watch "src/**" --ext "ts,tsx" --exec yarn test
Auto-publish on file change: nodemon --watch "src/**" --ext "ts" --exec "yalc publish --push --changed"

robertknight · 2022-02-18T08:12:46Z

Thanks for the contribution. I'm happy to take a look in detail when this is out of draft. Please let me know if you run into any roadblocks along the way.

kevinweber · 2022-02-18T08:34:21Z

@robertknight Great, thank you!
I actually have a pretty big question: How to make contexts work? …for both shallow and mount rendering?
Even though my current solution works for some cases where the wrapper is fairly dumb, it doesn't work for those who provide context. Below example gives me the following error:

could not find react-redux context value; please ensure the component is wrapped in a <Provider>

export const ProviderWithRouter = ({ children }: { children: ReactNode }) => (
  <Provider store={store}>
    <MemoryRouter>{children}</MemoryRouter>
  </Provider>
);

expect(
  mount(<Sidebar match={matched} location={location} history={history} />, {
    wrappingComponent: ProviderWithRouter,
  }),
).toHaveLength(1);

kevinweber · 2022-02-18T16:59:57Z

@robertknight Alighty, it is done! Please take a look. I'm quite happy with the solution so far and it works with context providers. I sampled this change with several tests in my org and nearly all of them pass. Some tests don't pass because they do weird/deprecated stuff and those tests need to be refactored anyway.

robertknight

I had a few small queries and comments. Generally this looks good though. Please let me know once you've had a chance to look through and make any changes, and then I'll get this shipped ASAP.

robertknight · 2022-02-18T19:46:09Z

src/Adapter.ts

+     * Using those props complicates a potential future migration to a different testing library.
+     * Instead, wrap a component like this:
+     * ```
+     * shallow(<Wrapper><Component/></Wrapper>)


Would tests then use wrapper.dive() to get at the wrapped component?

You're right. I verified that you need to add an additional .dive().

Before:

const wrapperProps = { wrappingComponent: MyTestContext.Provider, wrappingComponentProps: { value: { isMyTest: true } }, }; const wrapper: ShallowWrapper<{ isMyTest: boolean }> = shallow( <MyTitle>sup</MyTitle>, wrapperProps, ) .dive() .dive();

After:

const wrapper: ShallowWrapper<{ isMyTest: boolean }> = shallow( <MyTestContext.Provider value={true}> <MyTitle>sup</MyTitle> </MyTestContext.Provider>, wrapperProps, ) .dive() .dive() .dive();

robertknight · 2022-02-18T19:49:53Z

src/Adapter.ts

+    options?: ShallowRendererProps
+  ) => {
+    return {
+      RootFinder: RootFinder,


To check my understanding, RootFinder is essentially a marker to allow location of the wrapped component in the rendered tree?

If I understand the Enzyme source code correctly, it's purpose is to properly pass down context, see here https://github.com/enzymejs/enzyme/blob/57baba5ceaccec7a3ada40d7559f5bf71289cbe7/packages/enzyme/src/ShallowWrapper.js#L356 and here https://github.com/enzymejs/enzyme/blob/57baba5ceaccec7a3ada40d7559f5bf71289cbe7/packages/enzyme/src/ShallowWrapper.js#L322. I tried what happens if I don't insert RootFinder into wrapWithWrappingComponent and it didn't work.

robertknight · 2022-02-18T19:51:59Z

src/RootFinder.ts

@@ -0,0 +1,7 @@
+import { Component } from 'preact';
+
+export default class RootFinder extends Component {


It would be useful to have a brief comment explaining what RootFinder is used for.

robertknight · 2022-02-18T20:02:03Z

src/wrapWithWrappingComponent.ts

+
+  let nodeWithValidChildren = node;
+
+  if (typeof nodeWithValidChildren.props.children === 'string') {


To summarize, this is normalizing VNodes like { type: Widget, props: { children: 'test' }, ... } to { type: Widget, props: { children: ['test'] } }?

robertknight · 2022-02-18T20:03:31Z

src/wrapWithWrappingComponent.ts

+  if (typeof nodeWithValidChildren.props.children === 'string') {
+    // This prevents an error when `.dive()` is used:
+    // `TypeError: ShallowWrapper::dive() can only be called on components`
+    nodeWithValidChildren = Object.assign({}, nodeWithValidChildren);


Object.assign does a shallow clone so nodeWithValidChildren.props refers to the same object as node.props. You can use cloneElement to clone the node in order to modify the props.

robertknight · 2022-02-18T20:08:39Z

test/MountRenderer_test.tsx

+        .instance;
+      const expectedInstance = renderedTree[0].instance;
+
+      assert.equal(resultInstance.toString(), expectedInstance.toString());


Is the purpose of these asserts just to check that the rendered DOM structure is the same? If so comparing the outerHTML of the two instances with a single assert may be simpler.

Perfect 👍

kevinweber

@robertknight Thanks for the review! Very helpful. I followed up on your feedback. Additionally, I upgraded the Enzyme dependency to 3.11.0 because it allows diving into shallow components with context: enzymejs/enzyme#1966. Then I added three integration tests, one for mount and two for shallow.

One note: The change in this PR doesn't result in 100% matching behavior with the React adapter when wrappingComponent is used, in particular when it comes to shallow rendering. This might need follow-up enhancements to this library. So far, for the few cases where I found a shallow test to pass with React's but not Preact's adapter, a good solution was to migrate the test to using mount instead of shallow.

kevinweber · 2022-02-22T11:13:43Z

src/Adapter.ts

+    options?: ShallowRendererProps
+  ) => {
+    return {
+      RootFinder: RootFinder,


If I understand the Enzyme source code correctly, it's purpose is to properly pass down context, see here https://github.com/enzymejs/enzyme/blob/57baba5ceaccec7a3ada40d7559f5bf71289cbe7/packages/enzyme/src/ShallowWrapper.js#L356 and here https://github.com/enzymejs/enzyme/blob/57baba5ceaccec7a3ada40d7559f5bf71289cbe7/packages/enzyme/src/ShallowWrapper.js#L322. I tried what happens if I don't insert RootFinder into wrapWithWrappingComponent and it didn't work.

kevinweber · 2022-02-22T11:22:52Z

src/Adapter.ts

+     * Using those props complicates a potential future migration to a different testing library.
+     * Instead, wrap a component like this:
+     * ```
+     * shallow(<Wrapper><Component/></Wrapper>)


You're right. I verified that you need to add an additional .dive().

Before:

const wrapperProps = { wrappingComponent: MyTestContext.Provider, wrappingComponentProps: { value: { isMyTest: true } }, }; const wrapper: ShallowWrapper<{ isMyTest: boolean }> = shallow( <MyTitle>sup</MyTitle>, wrapperProps, ) .dive() .dive();

After:

const wrapper: ShallowWrapper<{ isMyTest: boolean }> = shallow( <MyTestContext.Provider value={true}> <MyTitle>sup</MyTitle> </MyTestContext.Provider>, wrapperProps, ) .dive() .dive() .dive();

kevinweber · 2022-02-22T11:32:04Z

src/RootFinder.ts

@@ -0,0 +1,7 @@
+import { Component } from 'preact';
+
+export default class RootFinder extends Component {


kevinweber · 2022-02-22T11:32:26Z

src/wrapWithWrappingComponent.ts

+
+  let nodeWithValidChildren = node;
+
+  if (typeof nodeWithValidChildren.props.children === 'string') {


kevinweber · 2022-02-22T11:40:19Z

src/wrapWithWrappingComponent.ts

+  if (typeof nodeWithValidChildren.props.children === 'string') {
+    // This prevents an error when `.dive()` is used:
+    // `TypeError: ShallowWrapper::dive() can only be called on components`
+    nodeWithValidChildren = Object.assign({}, nodeWithValidChildren);


kevinweber · 2022-02-22T11:44:53Z

test/MountRenderer_test.tsx

+        .instance;
+      const expectedInstance = renderedTree[0].instance;
+
+      assert.equal(resultInstance.toString(), expectedInstance.toString());


Perfect 👍

robertknight · 2022-02-24T07:19:24Z

So far, for the few cases where I found a shallow test to pass with React's but not Preact's adapter, a good solution was to migrate the test to using mount instead of shallow.

For new tests and when modernizing existing tests I would recommend using mount rendering, using module mocking or dependency injection if you want to stub out rendered components. I have a blog post with some notes on that.

robertknight

Looks good to me. Thank-you for the contribution, including the extensive tests.

robertknight · 2022-02-24T07:56:39Z

Published as v3.4.0.

kevinweber · 2022-02-24T09:10:14Z

test/integration_test.tsx

+      );
+    });
+
+    it('passes wrappingComponentProps to wrappingComponent', () => {


Note: A bit late because this PR is already merged but: I just realized that this test is basically a duplicate of the previous test. I used to run different testing logic in here but after some refactor I hadn't realized that it has become a redundant test. I'd suggest removing it in a future update.

Would you mind submitting a small follow-up PR?

kevinweber force-pushed the kw--wrapWithWrappingComponent branch from 4029646 to d135ae5 Compare February 17, 2022 19:03

kevinweber added 2 commits February 18, 2022 17:42

Support getWrappingComponentRenderer & wrapWithWrappingComponent

571d7a8

Add tests

829fae4

kevinweber force-pushed the kw--wrapWithWrappingComponent branch from 2225edb to 6f1bb9c Compare February 18, 2022 16:50

kevinweber marked this pull request as ready for review February 18, 2022 16:53

Remove context logic

7d7790d

kevinweber force-pushed the kw--wrapWithWrappingComponent branch from 6f1bb9c to 7d7790d Compare February 18, 2022 17:10

robertknight reviewed Feb 18, 2022

View reviewed changes

kevinweber added 3 commits February 22, 2022 13:55

PR feedback

720c15c

Add mount context test

f5ee93a

Update Enzyme dependency

2cdb301

kevinweber commented Feb 22, 2022

View reviewed changes

Add shallow context tests

9fc65e2

kevinweber force-pushed the kw--wrapWithWrappingComponent branch from 51ef2af to 9fc65e2 Compare February 22, 2022 15:18

kevinweber requested a review from robertknight February 23, 2022 23:04

robertknight approved these changes Feb 24, 2022

View reviewed changes

robertknight merged commit 4d51be8 into preactjs:master Feb 24, 2022

robertknight mentioned this pull request Feb 24, 2022

TypeError: your adapter does not support wrappingComponent. Try upgrading it! #99

Closed

kevinweber commented Feb 24, 2022

View reviewed changes

kevinweber mentioned this pull request Feb 24, 2022

Remove redundant test #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support wrappingComponent & wrappingComponentProps #157

Support wrappingComponent & wrappingComponentProps #157

kevinweber commented Feb 17, 2022 •

edited

Loading

robertknight commented Feb 18, 2022

kevinweber commented Feb 18, 2022 •

edited

Loading

kevinweber commented Feb 18, 2022 •

edited

Loading

robertknight left a comment

robertknight Feb 18, 2022

kevinweber Feb 22, 2022

robertknight Feb 18, 2022

kevinweber Feb 22, 2022

robertknight Feb 18, 2022

kevinweber Feb 22, 2022

robertknight Feb 18, 2022

kevinweber Feb 22, 2022

robertknight Feb 18, 2022

kevinweber Feb 22, 2022

robertknight Feb 18, 2022

kevinweber Feb 22, 2022

kevinweber left a comment •

edited

Loading

kevinweber Feb 22, 2022

kevinweber Feb 22, 2022

kevinweber Feb 22, 2022

kevinweber Feb 22, 2022

kevinweber Feb 22, 2022

kevinweber Feb 22, 2022

robertknight commented Feb 24, 2022

robertknight left a comment

robertknight commented Feb 24, 2022

kevinweber Feb 24, 2022

robertknight Feb 24, 2022

kevinweber Feb 24, 2022

		@@ -0,0 +1,7 @@
		import { Component } from 'preact';

		export default class RootFinder extends Component {


		let nodeWithValidChildren = node;

		if (typeof nodeWithValidChildren.props.children === 'string') {

Support wrappingComponent & wrappingComponentProps #157

Support wrappingComponent & wrappingComponentProps #157

Conversation

kevinweber commented Feb 17, 2022 • edited Loading

How was this tested?

robertknight commented Feb 18, 2022

kevinweber commented Feb 18, 2022 • edited Loading

kevinweber commented Feb 18, 2022 • edited Loading

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinweber left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Feb 24, 2022

robertknight left a comment

Choose a reason for hiding this comment

robertknight commented Feb 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinweber commented Feb 17, 2022 •

edited

Loading

kevinweber commented Feb 18, 2022 •

edited

Loading

kevinweber commented Feb 18, 2022 •

edited

Loading

kevinweber left a comment •

edited

Loading