Integrate new compat shallow renderer into Enzyme #233

andrewiggins · 2022-12-08T09:19:11Z

This PR adds a new CompatShallowRenderer that implements Enzyme's shallow renderer interface using the preact shallow rendering algorithm from #223. Usage of the new renderer is behind the useCompatShallowRendering flag.

index.d.ts

src/Adapter.ts

src/compat-shallow-renderer/PreactShallowRenderer.ts

test/shared.tsx

andrewiggins · 2022-12-08T09:58:20Z

test/shared.tsx

+    if (isNewShallowRender) {
+      allMethods.forEach(method => Test.prototype[method].resetHistory());
+    } else {
+      // Calling reset here not only resets call counts, but also any behaviors
+      // setup, such as `.returns(true)`. Calling reset here, removes our
+      // `.returns(true)` setup on shouldComponentUpdate, causing the method to
+      // return `undefined`. In shallow rendering, Enzyme spies on the return
+      // value of `sCU` and chooses to invoke lifecycle methods based on its
+      // return value. Because it returns `undefined` in this case, Enzyme skips
+      // invoking `cDU`.
+      //
+      // In the default shallow renderer, Preact invokes lifecycles. In this
+      // test, once `.reset()` is called, `.sCU` returns `undefined` and so
+      // enzyme does not invoke `cDU`. In a real test environment, cDU would be
+      // invoked twice. A simpler test that doesn't use spies can see this behavior.
+      allMethods.forEach(method => Test.prototype[method].reset());
+    }


More context: The new shallow renderer does not invoke componentDidUpdate or componentDidMount. It relies on Enzyme to handle that. Without the change to resetHistory(), it fails this test because when reset() is used, shouldComponentUpdate returns undefined which Enzyme interprets to mean to not invoked componentDidUpdate.

If I understand this test correctly, I think the intention is to only reset the call counts and not to clear all stub returns, hence my change to call resetHistory(). However this breaks the test for the old shallow renderer because now it demonstrates how componentDidUpdate will be invoked twice (once by Preact and once by Enzyme). So I left that behavior as is for now.

robertknight · 2022-12-09T14:32:17Z

I have approved the other PR. Please give me a ping when this is rebased.

…Preact10ShallowDiff

… test file

…antiated

andrewiggins · 2022-12-09T17:59:20Z

@robertknight PR is rebased and ready for review!

andrewiggins · 2022-12-16T00:45:35Z

@robertknight Updated per our comment thread about adding a compat sub package. I've moved all the compat shallow renderer related code into this sub package to better isolate and encapsulate that logic.

I think in my next PR I'll add a new test example that uses the compat renderer as a way to check that we are properly packaging and exposing the compat renderer so it can be included (i.e. I haven't missed any files in the root package.json files field and the compat/package.json is properly setup. (If you prefer, I can do that in this PR. My thought was that this PR is big enough already, but also I can see an argument for including that test in here :) )

robertknight · 2022-12-17T13:03:27Z

@robertknight Updated per our comment thread about adding a compat sub package. I've moved all the compat shallow renderer related code into this sub package to better isolate and encapsulate that logic.

Thanks @andrewiggins. I'm going to take a look this weekend.

robertknight

I had a few queries but this looks good in big picture terms. Are there other changes to come after this for the new shallow renderer?

CHANGELOG.md

compat/README.md

compat/package.json

compat/test/CompatShallowRenderer_test.tsx

compat/test/compat-shallow-renderer/PreactShallowRendererMemo_test.tsx

src/Adapter.ts

robertknight · 2022-12-19T13:23:00Z

test/integration_test.tsx

+      }
+
+      const appKey = 'app';
+      const wrapper = mount(<App key={appKey} />);


Searching the Enzyme codebase for references to getElement and getElements I only find it documented for the shallow rendering mode. Is this a mistake in the Enzyme docs?

Huh that's true. It appears to be. It is definitely implemented in their mount wrapper (source). As well as tested (source). I think I was researching what the right behavior here is for the shallow renderer and ended up writing the test for the mount renderer to compare and figured I'd add them. But if you don't think they are useful, happy to remove these.

I think the missing documentation is probably an oversight on Enzyme's side. Would you mind filing an issue against the Enzyme repo and referencing it here?

Opend! enzymejs/enzyme#2574

test/shared.tsx

Co-authored-by: Robert Knight <robertknight@gmail.com>

andrewiggins

Thanks for the review! I have no other big work items for the new shallow renderer planned at the moment, though I expect there will be incremental bug fixes as we start to use it at work.

I think with this change we have a good release candidate to get the first version of the new compat renderer out there.

compat/package.json

compat/README.md

compat/test/compat-shallow-renderer/PreactShallowRendererMemo_test.tsx

src/Adapter.ts

compat/test/CompatShallowRenderer_test.tsx

andrewiggins · 2022-12-19T18:53:13Z

test/integration_test.tsx

+      }
+
+      const appKey = 'app';
+      const wrapper = mount(<App key={appKey} />);


Huh that's true. It appears to be. It is definitely implemented in their mount wrapper (source). As well as tested (source). I think I was researching what the right behavior here is for the shallow renderer and ended up writing the test for the mount renderer to compare and figured I'd add them. But if you don't think they are useful, happy to remove these.

andrewiggins · 2022-12-19T19:07:33Z

src/Adapter.ts

@@ -60,6 +80,23 @@ export default class Adapter extends EnzymeAdapter {
    // Work around a bug in Enzyme where `ShallowWrapper.getElements` calls
    // the `nodeToElement` method with undefined `this`.
    this.nodeToElement = this.nodeToElement.bind(this);
+
+    if (this.preactAdapterOptions.ShallowRenderer) {
+      this.isFragment = node => node?.type === Fragment;


Yup! I wasn't entirely sure what would happen. If I enable it for all renderers, our test suite still passes. I believe it won't impact our existing renderers cuz we already remove fragments from the RST tree in rstNodeFromVNode. I was just being more conservative here, but it appears to be safe for us to enable for all renderers. Would you like me to do that?

robertknight · 2022-12-20T08:55:06Z

I think with this change we have a good release candidate to get the first version of the new compat renderer out there.

I agree. Let's get this landed. We don't have a convention in this repo for publishing pre-releases for early testing. What do other Preact projects do?

andrewiggins · 2022-12-20T19:50:23Z

I don't think we have any formal guidelines. In the past we've used -experimental, -alpha, -beta, and -rc tags when making major changes.

Actually, since everything here is behind flags, maybe we don't need to do a release candidate or pre-release. We could just do a minor release? Or would you rather do some sort of pre-release first to test changes? If so, perhaps we could use the -rc or -beta tag?

andrewiggins mentioned this pull request Dec 8, 2022

Moved integration shared tests into the shared test utils file #232

Merged

andrewiggins force-pushed the new-shallow-renderer branch from 945a33a to ae0590f Compare December 8, 2022 09:49

andrewiggins commented Dec 8, 2022

View reviewed changes

andrewiggins requested a review from robertknight December 8, 2022 22:21

Base automatically changed from shared-tests to master December 9, 2022 17:18

andrewiggins added 16 commits December 9, 2022 09:55

=== BEGIN new-shallow-renderer === Add new ShallowRenderer that uses …

83d7e66

…Preact10ShallowDiff

WIP: Copy shallow rendering tests into a new a new ShallowRendererNew…

c0927bb

… test file

Add CHANGELOG entry

1df223f

Add tests for getElement, children, and findWhere involving Fragments

78312d6

Fix lifecycle tests

9acb9d6

Improve simulateError in new shallow renderer

e0135ff

Modify props test to accommodate new shallow renderer behavior

14f93d3

Preserve children prop in new shallow renderer

03f9308

Add tests for methods on mount

893066b

Add test for shallow update, setState, and setProps

233282d

Flatten nested arrays in rstNodeFromElement

b382557

Only install shallow component hooks if PreactShallowRenderer is inst…

dda1540

…antiated

Implement displayNameOfNode if compat renderer is enabled

40b8b53

Remove old TODO

8a76c17

Ensure debounce hook is installed

7c7da56

Move isErrorBoundary to module scope

5e08b63

andrewiggins force-pushed the new-shallow-renderer branch from ae0590f to 5e08b63 Compare December 9, 2022 17:55

andrewiggins added 2 commits December 15, 2022 15:10

Add initial compat sub-package scaffolding

3c83ee6

Move compat renderer code to compat sub package

99ae9d7

robertknight approved these changes Dec 19, 2022

View reviewed changes

Apply suggestions from code review

6292c22

Co-authored-by: Robert Knight <robertknight@gmail.com>

andrewiggins added 3 commits December 19, 2022 09:34

Fix Enzyme capitalization

7b0f25d

Add comment explaining PreactShallowRendererMemo tests

a2ced75

Updates from PR

3c69d50

andrewiggins commented Dec 19, 2022

View reviewed changes

andrewiggins merged commit 9d7749b into master Dec 20, 2022

andrewiggins deleted the new-shallow-renderer branch December 20, 2022 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate new compat shallow renderer into Enzyme #233

Integrate new compat shallow renderer into Enzyme #233

andrewiggins commented Dec 8, 2022 •

edited

Loading

andrewiggins Dec 8, 2022

robertknight commented Dec 9, 2022

andrewiggins commented Dec 9, 2022

andrewiggins commented Dec 16, 2022

robertknight commented Dec 17, 2022

robertknight left a comment

robertknight Dec 19, 2022

andrewiggins Dec 19, 2022

robertknight Dec 20, 2022

andrewiggins Dec 20, 2022

andrewiggins left a comment

andrewiggins Dec 19, 2022

andrewiggins Dec 19, 2022

robertknight commented Dec 20, 2022

andrewiggins commented Dec 20, 2022

Integrate new compat shallow renderer into Enzyme #233

Integrate new compat shallow renderer into Enzyme #233

Conversation

andrewiggins commented Dec 8, 2022 • edited Loading

andrewiggins Dec 8, 2022

Choose a reason for hiding this comment

robertknight commented Dec 9, 2022

andrewiggins commented Dec 9, 2022

andrewiggins commented Dec 16, 2022

robertknight commented Dec 17, 2022

robertknight left a comment

Choose a reason for hiding this comment

robertknight Dec 19, 2022

Choose a reason for hiding this comment

andrewiggins Dec 19, 2022

Choose a reason for hiding this comment

robertknight Dec 20, 2022

Choose a reason for hiding this comment

andrewiggins Dec 20, 2022

Choose a reason for hiding this comment

andrewiggins left a comment

Choose a reason for hiding this comment

andrewiggins Dec 19, 2022

Choose a reason for hiding this comment

andrewiggins Dec 19, 2022

Choose a reason for hiding this comment

robertknight commented Dec 20, 2022

andrewiggins commented Dec 20, 2022

andrewiggins commented Dec 8, 2022 •

edited

Loading