guvenkaranfil commented 4 months ago

Summary

This draft PR is intended to start providing a way to detect unnecessary renders as described here

Test plan

I have added a test file called UnneccassaryRender.perf-test which is intended to unnecessarily render on the component and see the warning

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 6c6f88c27d3b922a9c26c37208d28cc77917fd09

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

| Name | Type | | --------------------------- | ----- | | reassure | Minor | | @callstack/reassure-compare | Minor | | @callstack/reassure-measure | Minor | | @callstack/reassure-cli | Minor | | @callstack/reassure-danger | Minor | | @callstack/reassure-logger | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

guvenkaranfil commented 4 months ago

I found something very interesting while testing the implementation @mdjastrzebski

For Unncessary Component test, wrapping state updating inside a timeout cause not finding the unnecessary renders instead of updating directly.

const handlePress = () => { setCount((c) => c + 1); };

with setTimeOut

const handlePress = () => { setTimeout(() => setCount((c) => c + 1), 10); };

guvenkaranfil commented 4 months ago

Another thing I came up with while testing is currently screen variable has been set in the outer scope.

On the first render screen variable is not set yet. On the second onRender callback screen variable is being set. That means first two onRender callbacks we can not do any comparison. So on the third callback can be comparasion because we have screen object and previously saved component representation

I am not sure I am missing any point you have described on the discussion issue @mdjastrzebski

mdjastrzebski commented 4 months ago

@guvenkaranfil good first step. As a next step you should probably modify measurePerformance function to capture the screen.toJSON() trees (only for RN!) to some array during the first run. Then after the 1st run compare the consecutive JSON trees to find out if they are the same.

guvenkaranfil commented 4 months ago

@mdjastrzebski I've just updated the implementation based on capturing component representations into an array and compare after.

Then after the 1st run compare the consecutive JSON trees to find out if they are the same.

Why not we would like to do comparison after 1st run instead of like end of the test like hasTooLateRender

mdjastrzebski commented 4 months ago

@guvenkaranfil The comparison can be made after all runs, that is not that important.

The important factors are:

Gather the toJSON tree during only the first count, and avoid capturing then during any subsequent runs in order not to affect the render times.
The first run is ideal here, as it will usually be discarded (warmupRuns: 1 is the default setup).