Add custom Preact shallow diff algorithm #223

andrewiggins · 2022-11-18T20:49:33Z

This PR implements a custom Preact shallow diff algorithm to better support shallow rendering and better match enzyme's React16Adapter's behavior. Below is a brief summary of the major changes.

This PR does not integrate the diff into any Enzyme adapter yet. That'll come in a follow up PR. I figured this PR was already very big and I didn't want to increase it more 😅

Preact10ShallowDiff

Adds Preact10ShallowDiff class modeled after the ReactShallowRenderer from the react-shallow-renderer package now maintained by enzyme. Preact10ShallowDiff Includes modified copies of Preact source to perform shallow rendering. The functions copied from preact and any modifications are commented as such.

New Babel transform step

Because the core diff algorithm is taken from Preact, the source of the function uses friendly internal property names (.e.g. _component). However, the built code that runs must use the mangled property names (e.g. __c) to interoperate with createElement and hooks. So I added an post build step that runs a babel transform on the Preact10ShallowDiff file to transform the property names to their mangled names.

New Tests

I've copied and TypeScript-ify the test suite from react-shallow-renderer. I'm running these tests in separate mocha run cuz this test suite requires using preact/compat and I didn't want to affect other tests. Also, the shallow diff algorithm should run without JSDOM so this separate mocha run doesn't setup JSDOM.

If you are curious, there is a separate commit for converting each test file from the original source to TypeScript and Preact if you want to see how I changed the tests.

test/Preact10ShallowDiff/Preact10ShallowDiffHooks_test.tsx

test/Preact10ShallowDiff/Preact10ShallowDiffMemo_test.tsx

test/Preact10ShallowDiff/Preact10ShallowDiff_test.tsx

test/Preact10ShallowDiff/utils.ts

scripts/transform-internal-properties.mjs

robertknight

I have had a look through this, and a skim through the copied tests. The main feedback I have is that, since this code is different from the rest of the codebase in a number of ways, and is copied directly from other codebases whose licenses we should reference, I suggest we put it in a separate subdirectory of src/.

I'm also thinking that in documentation for this package we should describe this alternate shallow renderer as existing primarily for compatibility with the React shallow renderer and to assist with migrating existing test suites, and not something that would be recommended for pure-Preact projects.

scripts/transform-internal-properties.mjs

src/Preact10ShallowDiff.ts

test/Preact10ShallowDiff/Preact10ShallowDiffMemo_test.tsx

Run Preact10ShallowDiff tests in their own mocha execution for two reasons: 1. Don't setup JSDOM. The shallow renderer should not have a dependency on a DOM-like environment 2. Preact10ShallowDiff tests are copied from React and so must be run with preact/compat. To avoid affecting other tests that are not suppose to test against preact/compat, we'll run these tests separatedly

… get working

…files

andrewiggins · 2022-11-24T01:36:21Z

@robertknight Updated with your suggestions. Let me know if I understood and followed through correctly.

In a follow up PR I'm going to add a new Enzyme renderer (like the existing ShallowRenderer.tsx) that uses this class to shallowly renderer components. In other words, a future PR will add the code that integrates the Preact10ShallowDiff class with enzyme behind a flag. In that PR, we'll work on the public facing name for this feature and documentation :)

robertknight

The changes look good, although I noted a mismatch in the names of the src/ vs test/ sub-directories. I also had a few more comments/questions.

src/compat-shallow-renderer/Readme.md

test/custom-shallow-renderer/Readme.md

test/custom-shallow-renderer/LICENSE

src/compat-shallow-renderer/Preact10ShallowDiff.ts

src/compat-shallow-renderer/preact10-src.ts

robertknight · 2022-11-25T05:59:09Z

test/custom-shallow-renderer/utils.ts

+
+// These tests are from react-shallow-renderer which uses Jest for testing, so
+// here is a lightweight adapter around `chai` to give it the same API as Jest's
+// `expect` so we don't have to rewrite all the `expect` statements to `assert`


Chai does have an expect API. Can that be used directly or does it differ from Jest's expect API significantly?

It does differ. Chai's expect API would be something like expect(actual).to.equal(expected) whereas jest would be expect(actual).toBe(expected).

I thought about wrapping using Chai's expect in this helper but I figured it would be simpler to just stick with the assertion style we are already using

robertknight · 2022-11-25T06:02:53Z

test/custom-shallow-renderer/utils.ts

+      // The Preact diff uses `__v` to shortcut diffing VNodes that haven't
+      // changed since creation (it treats VNodes as immutable). So when Preact
+      // checks if `oldVNode.__v == newVNode.__v`, setting `__v` to NaN will
+      // always return false, per the rules of JavaScript (NaN !== NaN in JS).


I presume React doesn't have this short-circuiting behavior?

I'm not sure. It might have something similar but I think it does it it in a different way that doesn't rely on private VNode properties (it has been a while since I looked).

robertknight · 2022-11-25T06:04:49Z

test/custom-shallow-renderer/utils.ts

+
+      // Override the vnodeId (`__v`) to NaN so that we can compare VNodes in
+      // tests and verify expected VNode output. We choose NaN here because it
+      // successfully threads the desired behavior we want for these tests.


To check my understanding here, what you're doing is disabling a Preact optimization in order to make Preact vnodes behave more like React vnodes and thereby enable a test suite originally written for React to pass?

Yea, close. More precisely, I'm' trying to enable tests that do expect(<div />).toBe(<div />) which some our tests do. Our tests instead use the stripInternalVNodeFields to get around this __v issue. I went with this approach instead of using stripInternalVNodeFields just cuz it would be an easier change given the number of tests that compare VNodes.

It does disable a Preact optimization which is unfortunate though. The explicit goal isn't to make Preact VNodes behave like React VNodes but this change does make that more true.

Actually, an idea a coworker had (we also have quite a few tests that do expect(<div />).toBe(<div />) at work) is to make all of the internal VNodes field non-enumerable. Doing this would remove the need for the stripInternalVNodeFields helper altogether.

I was thinking about doing that in a separate PR but could bring it here if you wanted since we are talking about it. Here is the relevant commit.

Okay, turns out my "make all internal fields non-enumerable" doesn't work as expected. Preact does iterate over internal VNode props to clone VNodes when rerendering so they need to be enumerable.

However, the __v field is specially handled so we can make just that property non-enumerable. I didn't see a place where that would break Preact but I think would enable doing things like expect(<div />).toEqual(<div />). Again, I could do this in a follow up PR or bring it here. My default would be to do it in a separate PR.

andrewiggins

Thanks for the thorough review and additional comments! Keep 'em coming :)

andrewiggins · 2022-11-25T19:53:26Z

test/custom-shallow-renderer/utils.ts

+
+// These tests are from react-shallow-renderer which uses Jest for testing, so
+// here is a lightweight adapter around `chai` to give it the same API as Jest's
+// `expect` so we don't have to rewrite all the `expect` statements to `assert`


It does differ. Chai's expect API would be something like expect(actual).to.equal(expected) whereas jest would be expect(actual).toBe(expected).

I thought about wrapping using Chai's expect in this helper but I figured it would be simpler to just stick with the assertion style we are already using

test/custom-shallow-renderer/LICENSE

src/compat-shallow-renderer/Preact10ShallowDiff.ts

src/compat-shallow-renderer/preact10-src.ts

andrewiggins · 2022-11-25T20:44:34Z

test/custom-shallow-renderer/utils.ts

+      // The Preact diff uses `__v` to shortcut diffing VNodes that haven't
+      // changed since creation (it treats VNodes as immutable). So when Preact
+      // checks if `oldVNode.__v == newVNode.__v`, setting `__v` to NaN will
+      // always return false, per the rules of JavaScript (NaN !== NaN in JS).


I'm not sure. It might have something similar but I think it does it it in a different way that doesn't rely on private VNode properties (it has been a while since I looked).

andrewiggins · 2022-11-25T20:50:07Z

test/custom-shallow-renderer/utils.ts

+
+      // Override the vnodeId (`__v`) to NaN so that we can compare VNodes in
+      // tests and verify expected VNode output. We choose NaN here because it
+      // successfully threads the desired behavior we want for these tests.


Yea, close. More precisely, I'm' trying to enable tests that do expect(<div />).toBe(<div />) which some our tests do. Our tests instead use the stripInternalVNodeFields to get around this __v issue. I went with this approach instead of using stripInternalVNodeFields just cuz it would be an easier change given the number of tests that compare VNodes.

It does disable a Preact optimization which is unfortunate though. The explicit goal isn't to make Preact VNodes behave like React VNodes but this change does make that more true.

andrewiggins · 2022-11-25T20:53:44Z

test/custom-shallow-renderer/utils.ts

+
+      // Override the vnodeId (`__v`) to NaN so that we can compare VNodes in
+      // tests and verify expected VNode output. We choose NaN here because it
+      // successfully threads the desired behavior we want for these tests.


Actually, an idea a coworker had (we also have quite a few tests that do expect(<div />).toBe(<div />) at work) is to make all of the internal VNodes field non-enumerable. Doing this would remove the need for the stripInternalVNodeFields helper altogether.

I was thinking about doing that in a separate PR but could bring it here if you wanted since we are talking about it. Here is the relevant commit.

andrewiggins · 2022-11-25T20:57:26Z

(side note: I've been resolving comments involving code changes that I do, as a way for me to track that I've done them. If that makes it more difficult to follow up and track your comments and my replies, let me know. I can stop doing that 😅)

robertknight · 2022-12-05T08:46:25Z

Sorry to keep you waiting on this Andre. I'll be taking another look today.

robertknight

Hello Andre,

Thanks for your patience with this. The organizational changes for src/compat-shallow-renderer and READMEs look good. I have largely skimmed the source files which have been copied and adapted from the corresponding react-shallow-renderer sources. When we come to integrate this into the adapter, I would suggest setting things up in such a way that the compat-shallow-renderer code is only included in test bundles for projects that actually use it.

robertknight · 2022-12-07T10:11:10Z

package.json

+    "test": "nyc mocha -r build/test/init.js build/test/*.js && nyc mocha build/test/compat-shallow-renderer",
+    "posttest": "rm build/package.json",
+    "pretest-cjs": "yarn build-cjs",
+    "test-cjs": "nyc mocha -r build-cjs/test/init.js build-cjs/test/*.js && nyc mocha build-cjs/test/compat-shallow-renderer",


I wonder if we could think about dropping support for the CommonJS build at some point and going ESM-only.

Yup. Perhaps in the next major?

robertknight · 2022-12-07T10:17:01Z

src/preact10-internals.ts

+import { Fragment } from 'preact';
+
+// This makes enzyme debug output easier to work with by giving Fragments a name that isn't minified
+Fragment.displayName = 'Fragment';


Hmm, it might be better to put this in one of the entry points for this library to avoid a potentially surprising side-effect of importing this package.

Actually, yea good call. Since this is a side-effect, I'm going to remove this from the src and put in the test/compat-shallow-renderer/utils.ts for now to avoid the side-effect altogether at the moment. I think I'll add it only if the compat shallow renderer is used so it only gets installed if the compat behavior is opted into. That way the side-effect is considered part of compat side-effects only.

I'll do that in the next PR where I integrate the shallow renderer into enzyme and determine how to only include the files if they are opted into (so side-effects like this one are opted into)

robertknight · 2022-12-07T10:20:46Z

test/compat-shallow-renderer/utils.ts

+  return matchers;
+}
+
+export function installVNodeTestHook() {


Am I correct in understanding that once this test hook is installed in a process, it will affect all tests run afterwards? So this means that we need to ensure that the shallow renderer tests are run in a separate process from the other tests.

Yup, that's correct. Agreed! The compat-shallow-renderer tests are running in their own nyc call in the package.json: nyc mocha -r build/test/init.js build/test/*.js && nyc mocha build/test/compat-shallow-renderer. I had originally done that cuz I wanted these tests to run without jsdom and so that the preact/compat usage wouldn't affect other tests. But like you suggest, this vnode option hook is another good reason to have these tests run separately.

robertknight · 2022-12-07T10:26:45Z

test/compat-shallow-renderer/PreactShallowRendererExtra_test.tsx

+    ].forEach(expectLifecycleNotCalled(SomeComponent));
+  });
+
+  it('changes between two memoed components', () => {


Can you clarify the significance of these tests? Is this to make sure memo doesn't share any state between different memoed components?

Sort of. The shallow renderer has special code to handle how Preact supports memo. These tests capture some edge cases my original handling of memo missed. More specially, this test ensures line 194 in PreactShallowRenderer.ts (this._memoElement?.type === element.type) detects two different memo components. Two different memo components should fail this check (i.e. not have equal types) so the renderer will unmount the previous component and mount the new one.

robertknight · 2022-12-07T10:55:48Z

test/compat-shallow-renderer/PreactShallowRendererMemo_test.tsx

@@ -0,0 +1,1606 @@
+/**


I see this is adapted from https://github.com/enzymejs/react-shallow-renderer/blob/master/src/__tests__/ReactShallowRendererMemo-test.js upstream. This does seem like a lot of tests to cover the memo API though.

Haha yea, agreed. I was surprised as well. It looks like many of the tests from PreactShallowRenderer_test.tsx are duplicated here but using memo. I suspect this is because the react shallow renderer has to implement the memo behavior itself (react-shallow-renderer runs comparer function).

andrewiggins

Thanks for taking the time to look again and all the great feedback. I really appreciate it <3

andrewiggins · 2022-12-07T19:21:37Z

package.json

+    "test": "nyc mocha -r build/test/init.js build/test/*.js && nyc mocha build/test/compat-shallow-renderer",
+    "posttest": "rm build/package.json",
+    "pretest-cjs": "yarn build-cjs",
+    "test-cjs": "nyc mocha -r build-cjs/test/init.js build-cjs/test/*.js && nyc mocha build-cjs/test/compat-shallow-renderer",


Yup. Perhaps in the next major?

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

test/compat-shallow-renderer/PreactShallowRendererExtra_test.tsx

+    ].forEach(expectLifecycleNotCalled(SomeComponent));
+  });
+
+  it('changes between two memoed components', () => {


Sort of. The shallow renderer has special code to handle how Preact supports memo. These tests capture some edge cases my original handling of memo missed. More specially, this test ensures line 194 in PreactShallowRenderer.ts (this._memoElement?.type === element.type) detects two different memo components. Two different memo components should fail this check (i.e. not have equal types) so the renderer will unmount the previous component and mount the new one.

andrewiggins · 2022-12-07T19:57:55Z

test/compat-shallow-renderer/utils.ts

+  return matchers;
+}
+
+export function installVNodeTestHook() {


Yup, that's correct. Agreed! The compat-shallow-renderer tests are running in their own nyc call in the package.json: nyc mocha -r build/test/init.js build/test/*.js && nyc mocha build/test/compat-shallow-renderer. I had originally done that cuz I wanted these tests to run without jsdom and so that the preact/compat usage wouldn't affect other tests. But like you suggest, this vnode option hook is another good reason to have these tests run separately.

andrewiggins · 2022-12-07T20:08:27Z

test/compat-shallow-renderer/PreactShallowRendererMemo_test.tsx

@@ -0,0 +1,1606 @@
+/**


Haha yea, agreed. I was surprised as well. It looks like many of the tests from PreactShallowRenderer_test.tsx are duplicated here but using memo. I suspect this is because the react shallow renderer has to implement the memo behavior itself (react-shallow-renderer runs comparer function).

andrewiggins · 2022-12-07T20:16:53Z

src/preact10-internals.ts

+import { Fragment } from 'preact';
+
+// This makes enzyme debug output easier to work with by giving Fragments a name that isn't minified
+Fragment.displayName = 'Fragment';


Actually, yea good call. Since this is a side-effect, I'm going to remove this from the src and put in the test/compat-shallow-renderer/utils.ts for now to avoid the side-effect altogether at the moment. I think I'll add it only if the compat shallow renderer is used so it only gets installed if the compat behavior is opted into. That way the side-effect is considered part of compat side-effects only.

I'll do that in the next PR where I integrate the shallow renderer into enzyme and determine how to only include the files if they are opted into (so side-effects like this one are opted into)

andrewiggins · 2022-12-07T20:22:31Z

Per usual, if you think of something else you want to comment on, feel free to leave the comment here. Just @-mention me and I'll address it in a follow up PR :)

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 `shallowRenderer` flag. * Add new ShallowRenderer that uses Preact10ShallowDiff * WIP: Copy shallow rendering tests into a new a new ShallowRendererNew test file * Add CHANGELOG entry * Add tests for getElement, children, and findWhere involving Fragments * Fix lifecycle tests * Improve simulateError in new shallow renderer * Modify props test to accommodate new shallow renderer behavior * Preserve children prop in new shallow renderer * Add tests for methods on mount * Add test for shallow update, setState, and setProps * Flatten nested arrays in rstNodeFromElement * Only install shallow component hooks if PreactShallowRenderer is instantiated * Implement displayNameOfNode if compat renderer is enabled * Remove old TODO * Ensure debounce hook is installed * Move isErrorBoundary to module scope * Add initial compat sub-package scaffolding * Move compat renderer code to compat sub package * Apply suggestions from code review Co-authored-by: Robert Knight <robertknight@gmail.com> * Fix Enzyme capitalization * Add comment explaining PreactShallowRendererMemo tests * Updates from PR Co-authored-by: Robert Knight <robertknight@gmail.com>

andrewiggins commented Nov 18, 2022

View reviewed changes

scripts/transform-internal-properties.mjs Show resolved Hide resolved

andrewiggins force-pushed the preact-shallow-diff branch 4 times, most recently from 370d499 to 51953bc Compare November 21, 2022 19:56

andrewiggins requested a review from robertknight November 21, 2022 19:57

andrewiggins force-pushed the preact-shallow-diff branch 3 times, most recently from 67e2485 to d048d6e Compare November 22, 2022 09:33

robertknight reviewed Nov 23, 2022

View reviewed changes

andrewiggins added 13 commits November 23, 2022 12:08

Add Preact10ShallowDiff

841797f

Add script to transform internal preact property names

27441fa

Add react-shallow-renderer tests unmodified but commented out for now

cb5751a

Enable Preact10ShallowDiffHooks_test

f443edb

Fix up memo tests but skip them for now as it requires more effort to…

f8656dd

… get working

Remove commitQueue field

5597b20

Simplify memo handling and use bailout symbol to signal bailout

91b80b1

Skip tests that Preact doesn't support in shallow diff memo test suite

4e8628f

Get memo shallow diff tests working

f56cb94

Add extra shallow diff tests

9f45170

Further simplify shallow memo handling and add comments

70863a8

Rename bailoutSymbol to skipUpdateSymbol

b88b14e

andrewiggins force-pushed the preact-shallow-diff branch from d048d6e to b88b14e Compare November 23, 2022 20:09

andrewiggins added 2 commits November 23, 2022 14:18

Use mangle.json from Preact source for property name mangling

696ccc8

Clean up comment

5ad24fa

andrewiggins force-pushed the preact-shallow-diff branch from 38dd201 to 5ad24fa Compare November 23, 2022 22:18

andrewiggins added 2 commits November 23, 2022 14:41

Move Preact10ShallowDiff into compat-shallow-renderer sub directory

b08f21d

Further separate src from other packages and add readmes and license …

5a80845

…files

robertknight reviewed Nov 25, 2022

View reviewed changes

andrewiggins added 2 commits November 25, 2022 11:57

Delete readme with incorrect capitalization

dfe31b0

Bring back READMEs with correct capitalization

c21e226

andrewiggins commented Nov 25, 2022

View reviewed changes

andrewiggins added 3 commits November 25, 2022 12:59

Fix folder name

177256e

Rename Preact10ShallowDiff to PreactShallowRenderer

e13c8b2

Rename preact10-src to preact10-shallow-diff

4cce670

andrewiggins force-pushed the preact-shallow-diff branch from ec7af09 to 4cce670 Compare November 25, 2022 21:00

robertknight approved these changes Dec 7, 2022

View reviewed changes

Move Fragment.displayName to test utils

6e05c49

andrewiggins commented Dec 7, 2022

View reviewed changes

andrewiggins merged commit 68b489f into master Dec 7, 2022

andrewiggins deleted the preact-shallow-diff branch December 7, 2022 20:22

andrewiggins mentioned this pull request Dec 8, 2022

Integrate new compat shallow renderer into Enzyme #233

Merged

Add custom Preact shallow diff algorithm #223

Add custom Preact shallow diff algorithm #223

Conversation

andrewiggins commented Nov 18, 2022 • edited Loading

Preact10ShallowDiff

New Babel transform step

New Tests

robertknight left a comment

Choose a reason for hiding this comment

andrewiggins commented Nov 24, 2022

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

andrewiggins Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewiggins 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

andrewiggins Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

andrewiggins commented Nov 25, 2022

robertknight commented Dec 5, 2022

robertknight 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

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

andrewiggins 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

andrewiggins commented Dec 7, 2022

andrewiggins commented Nov 18, 2022 •

edited

Loading

andrewiggins Nov 25, 2022 •

edited

Loading

andrewiggins Nov 25, 2022 •

edited

Loading

robertknight left a comment •

edited

Loading