Made a few changes to fetchFriendFIDs in NeynarClient. These changes are mostly improvements to readability + organization of the function. I also implemented some basic error handling to catch any failed fetch requests
Details
Details
- Reviewers
atul inka ashoat - Commits
- rCOMMfe5a6721ac17: [lib] refine fetchFriendFIDs function
Confirmed that everything still works for fetching farcaster data
- If there are no results (response is an empty array) the cursor is null and the loop does not run again
- If there are results, but the query limit is very small (i tested with setting the limit to 1) then cursor is set and the loop keeps on running until cursor is null and then the loop stops, and I get sent back all the relevant farcaster data
- If the number of results is less than the query limit then the cursor is null and the loop only runs once and I get sent back all the relevant farcaster data.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/utils/neynar-client.js | ||
|---|---|---|
| 17 ↗ | (On Diff #39184) | Noticed that the cursor field could potentially be null, so updated the type to reflect that   |
| 85 ↗ | (On Diff #39184) | Thought it was more intuitive to have the loop check be based on if the paginationCursor is set or not. The paginationCursor will only be set if there is more data that needs to be fetched (the number of results > the query limit passed in the params) so in that case we should re run this loop to get the rest of the results |
Comment Actions
In this case since I was the author of most of this code, it probably would've been a good idea to put me on the review
| lib/utils/neynar-client.js | ||
|---|---|---|
| 49 ↗ | (On Diff #39184) | I don't think we need to return $ReadOnlyArray. We're constructing a new array here and once we return it to the caller, they can do whatever they want with it. If they want to, for instance, append the viewer's FID to the array, I don't see anything wrong with that |
| 83 ↗ | (On Diff #39184) | You are introducing a new issue here... you're trying to cast error as a string but that might not work. You should be using getMessageForException But at a higher level, it's really not clear to me what you're achieving by catching an error and throwing a different one. This is just adding a level of indirection. What is gained here? |
| 85 ↗ | (On Diff #39184) | I thought I checked and there were some subtle cases where that wasn't true. I recall initially doing that but changing the condition to users.length < fetchFollowerLimit at a later point after some testing |
| lib/utils/neynar-client.js | ||
|---|---|---|
| 85 ↗ | (On Diff #39184) | It's possible Neynar has changed since I played around with it... eg. they might have had a bug where they were always sending back a cursor, so I made it cursor: string and had the loop be based on the number of results... now that I think about it, that seems likely |
| lib/utils/neynar-client.js | ||
|---|---|---|
| 83 ↗ | (On Diff #39184) | I guess you're adding a message here If you're replacing the exception I would at least console.log(error); here so we can get the original thing |
| lib/utils/neynar-client.js | ||
|---|---|---|
| 49 ↗ | (On Diff #39184) | Got it thank you for explaining, I thought using "read only" was best practice but your explanation above makes sense and I will try to use this thinking to determine whether or not to use "read only" in the future |