🔬 Make useRawLogs more efficient and responsive #1151
Conversation
TylerEther
commented
Apr 21, 2024
•
TylerEther
commented
- Makes use of isStatic query param
- Makes use of refresh query param and global config
- Clears logs when there's a change in the filter to minimize the amount of time invalid old logs are returned
- Prevents some race conditions
- It no longer fetches logs with every re-render
- Makes use of isStatic query param - Makes use of refresh query param and global config - Clears logs when there's a change in the filter to minimize the amount of time invalid old logs are returned - Prevents some race conditions - It no longer fetches logs with every re-render - Filter can no longer be a promise - reduces unnecessary complexity
🦋 Changeset detectedLatest commit: af09026 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
There was a problem hiding this comment.
Hey @TylerEther,
thank you for contributing to the repo.
I have a question regarding the PR
Removing Promise from useRawLogs does simplify the code however introduces breaking changes so we would have to release this PR as new major version. Is there a way you could keep the function parameters unchanged while introducing the rest of your changes ?
| } | ||
|
|
||
| useEffect(() => { | ||
| useMemo(() => { |
There was a problem hiding this comment.
Also shouldn't useEffect behave the same as useMemo here. Why the change ?
There was a problem hiding this comment.
Good catch. I was playing around with what worked best. I've switched back to useEffect as they behave the same and to also handle cleanup procedures.
- Switch back to allowing filter promises - Switch back to useEffect to update the logs - Shallow copy filter data before the async getLogs to prevent data mismatch making logs stale - Prevent changing state after component unmount - Use useRef to track loading status
Good point. It was tricky, but I found a way to handle filter promises so I added that back. |
|
I've introduced a helper hook, |
| // it's just the same object. No need to compare. | ||
| return true | ||
|
|
||
| if (obj1 == null) return obj1 == null |
There was a problem hiding this comment.
| if (obj1 == null) return obj1 == null | |
| if (obj1 == null) return obj2 == null |
I believe this is a typo
otherwise deepEqual(null, 2) would return true
There was a problem hiding this comment.
I don't know if this is intended behaviour but it seems it would return
deepEqual(null, undefined) as true
| if (obj1 === obj2) | ||
| // it's just the same object. No need to compare. | ||
| return true |
There was a problem hiding this comment.
| if (obj1 === obj2) | |
| // it's just the same object. No need to compare. | |
| return true | |
| if (obj1 === obj2) return true |
I believe it's self explanatory enough no need to add comment
| let _filter: Filter | FilterByBlockHash | Falsy = undefined | ||
|
|
||
| if (filter instanceof Promise) { | ||
| _filter = await filter | ||
| } else { | ||
| _filter = filter | ||
| } |
There was a problem hiding this comment.
| let _filter: Filter | FilterByBlockHash | Falsy = undefined | |
| if (filter instanceof Promise) { | |
| _filter = await filter | |
| } else { | |
| _filter = filter | |
| } | |
| let _filter: Filter | FilterByBlockHash | Falsy = await filter |
If you await non promise value it will just return it as value otherwise it will resolve promise
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await#return_value
|
Thanks for the review @Szymx95. I've added tests for everything, made some improvements, and additionally tested the latest changes with my web app. Everything functions well. |
|
Looks good could you rebase to current master branch ? |
Done @Szymx95 |
TylerEther
changed the title
