chore: enable rules of hooks for RN chat SDK

[isekovanic]

🎯 Goal

As @santhoshvai pointed out in this Slack thread, the Rules of Hooks eslint plugin has been disabled for the Chat SDK. We want to re-enable it so that future development follows these rules intrinsically.

🛠 Implementation details

This is mostly a port of GetStream/stream-chat-react#2244 (thanks @myandrienko !) to our SDK.

Brief rundown of the steps:

• Enable the react-hooks/exhaustive-deps plugin
• Run yarn eslint 'src/**/*.{js,ts,tsx,md}' -f json -o lint.json in all directories for which the plugin was enabled
• Execute the script mentioned in the PR above (be mindful to change const json = require('./lint.json'); to whichever path your lint.json files got generated at)
• Run yarn prettier --write to fix all of the formatting issues
• Finally, run yarn eslint again to make sure that no warnings pop up after the script had been run

Care that prettier might also do some other changes in additional files - everything was checked by hand as well after the script was run and odd edge cases were handled adhoc (in my case they were only prettier ones).

🎨 UI Changes

iOS

Before	After

Android

Before	After

🧪 Testing

☑️ Checklist

• I have signed the Stream CLA (required)
• PR targets the develop branch
• Documentation is updated
• New code is tested in main example apps, including all possible scenarios
  • SampleApp iOS and Android
  • Expo iOS and Android

[Stream-SDK-Bot]

SDK Size

title	develop	branch	diff	status
js_bundle_size	438.4580078125 KB	438 KB	0 B	🟢

[oliverlaz]

are these false positives? curious why the script detects these as hook deps :)

[isekovanic]

Not too sure why the eslint-disable-next-line happens here either (instead of useMemo itself), but they're essentially here because useMemo is invoked every time and these aren't memoized at all (defined inline, will change on every render), while consuming resources as React memoization does. It's probably because if we want to actually memoize these, we should either:

• Move the function definitions inside of the useMemo (and make it depend on viewportDimensions.height/viewportDimensions.width
• Wrap the definitions within useCallback (with their respective dependencies of width/height), while getting rid of useMemo altogether

In my opinion these shouldn't be subject to memoization at all since I'm pretty sure the memoization itself is more expensive than directly setting them to some state (within the event listener) and just passing them like so. But we can do it incrementally and see what sticks.

[isekovanic]

That is, assuming some downstream project doesn't actually rely on the fact that these aren't memoized :D that might be a bit tricky

[oliverlaz]

I think it would be best if we remove useMemo(() => ({ vh, vw }), [vh, vw]); and return directly.

[isekovanic]

Yep, after I'm done with some other things I'll jump into taking care of these easier low hanging fruits which should improve performance a bit.

[santhoshvai]

nice catch here, this useMemo does nothing here, useCallback for both vw and vh and then remove useMemo here

I think vw and vh is used in multiple React.Memos

[isekovanic]

Yep, I'll have to look how far the non-memoization cascades here. Also I hope that some downstream implementations do not do not rely on the fact that vh and vw aren't memoized and that we break them. Should we add this somewhere ?

[stream-ci-bot]

🎉 This PR is included in version 5.36.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀