Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔬 Make useRawLogs more efficient and responsive #1151

Merged
merged 25 commits into from
May 27, 2024

Conversation

TylerEther
Copy link
Contributor

@TylerEther TylerEther commented Apr 21, 2024

  • 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
Copy link

changeset-bot bot commented Apr 21, 2024

🦋 Changeset detected

Latest commit: af09026

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

This PR includes changesets to release 3 packages
Name Type
@usedapp/core Patch
@usedapp/coingecko Patch
@usedapp/testing Patch

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

Copy link
Contributor

@Szymx95 Szymx95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also shouldn't useEffect behave the same as useMemo here. Why the change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@TylerEther
Copy link
Contributor Author

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 ?

Good point. It was tricky, but I found a way to handle filter promises so I added that back.

@TylerEther
Copy link
Contributor Author

I've introduced a helper hook, useResolvedFilter and helper functions, deepEqual and isPrimitive. Let me know if you'd like me to extract these into a common helper file or if you'd like some unit tests for them.

Copy link
Contributor

@Szymx95 Szymx95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking your time and reworking the solution.

I've found a few things in deepEqual that I think might lead to unintended behaviours. It would be great if you could extract those helpers and write some unit tests.

// it's just the same object. No need to compare.
return true

if (obj1 == null) return obj1 == null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is intended behaviour but it seems it would return
deepEqual(null, undefined) as true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Comment on lines 11 to 13
if (obj1 === obj2)
// it's just the same object. No need to compare.
return true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Comment on lines 49 to 55
let _filter: Filter | FilterByBlockHash | Falsy = undefined

if (filter instanceof Promise) {
_filter = await filter
} else {
_filter = filter
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean

@TylerEther
Copy link
Contributor Author

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.

@Szymx95
Copy link
Contributor

Szymx95 commented May 13, 2024

Looks good could you rebase to current master branch ?

@TylerEther
Copy link
Contributor Author

Looks good could you rebase to current master branch ?

Done @Szymx95

@TylerEther TylerEther changed the title Make useRawLogs more efficient and responsive 🔬 Make useRawLogs more efficient and responsive May 15, 2024
@nezouse nezouse merged commit fb44f98 into TrueFiEng:master May 27, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants