Comparing: af73043...8797f17

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Significant size changes

Includes any change greater than 0.2%:

Generated by 🚫 dangerJS against 8797f17

Fascinating.

It seems worrying that we get to such high numbers. Does this also mean we're doing a ton of work and are slow in this case? Is this case pathological in some particular way? Can we think of any way to do less work for it?

cc @calebmer you might find this interesting

Work Context

Adding some screenshots from when I was working just to add some context. This first screenshot is before I added BigInts. The pathsFromStartToEnd and allPathsFromStartToEnd are the values that are being compared in the conditional for the error.

These are the values from the same code but with BigInts.

The code is using a recursive algorithm with time O(log n). I added a console log to the function to see the value at each step of recursion for countPathsFromStart.

Concluding Thoughts

I do not see any way to get around having very large integers if a user has complex code with a lot of conditionals. I have not done a deep dive into the recursive algorithm, but it seems well designed and runs in O(log n). As we have to make sure the hook is reachable from the initial segment and every final segment, I believe we will have to traverse the code somehow.

I'm wondering if there is a way to do this with simple multiplication. If there are 4 simple if else statements before a hook, then there are 2^4 pathsFromStart. We'd have to distinguish the different types of conditionals somehow, which the current recursive algorithm already takes care of.

Can you do some kind of a stress test to see how this change affects performance for the common cases? I'm not sure if there are any perf pitfalls to using BigInts.

Setup

I took the eslint-hook-bug repository and made some adjustments. I copied app/foo.tsx (the file that previously failed because the ints were too big) and copied it to fail.tsx. I then removed a couple of the conditionals from app/foo.tsx so there would be no errors when run with the current version. Finally, I copied app/foo.tsx 100 times into the app folder for the stress test.

Stress Test

I checked out the main branch and ran yarn build eslint-plugin-react-hooks to simulate the current environment.

Eslint comes with feature that shows you how long each run takes. The command 'TIMING=1 yarn eslint app/*' will run the rule on every file in app and give us the total time eslint took to run. I ran the command 10 times and sent the output to main_runs.txt. I then ran the same command but with fail.tsx and also sent that output to main_runs.txt just so we can see the failure.
main_runs.txt

I then switched over to the branch that with BigInts and reran yarn build.

I then ran the same TIMING=1 yarn eslint app/* command 10 times and sent the output to big_int_runs.txt. I then ran it with fail.tsx to watch it pass this time.
big_int_runs.txt

Concluding Thoughts

The runtimes seemed very similar to me. If you would also like me to check the memory usage difference, let me know. That might take longer though as I'd have to do a lot of research on how to do that.

Thanks for checking. Would you mind fixing the lint issue? I think you need to find the lint config we use for builds and add BigInt to known globals.

Looks like your commits aren't attributed to your GH user (maybe different email). Would you like to fix them up so that you appear as a contributor after merge? I'm not sure how this works.

You were right I had an old email in there. Don't worry about it though.

Thanks!