Page MenuHomePhabricator

[lib] Introduce useENSNames to fetch more than one ENS name at a time
ClosedPublic

Authored by ashoat on Jan 27 2023, 3:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Sep 30, 4:49 AM
Unknown Object (File)
Thu, Sep 19, 7:00 PM
Unknown Object (File)
Wed, Sep 18, 8:50 PM
Unknown Object (File)
Wed, Sep 18, 4:55 PM
Unknown Object (File)
Wed, Sep 18, 1:12 PM
Unknown Object (File)
Wed, Sep 18, 11:33 AM
Unknown Object (File)
Wed, Sep 18, 9:35 AM
Unknown Object (File)
Wed, Sep 18, 9:35 AM
Subscribers

Details

Summary

Also rewrite useStringForUser to call into useENSNames.

Test Plan

Tested existing calls of useStringForUser, both when logged in as an ENS user and when not

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  1. Don't check isViewer in useENSNames, that check is handled in useStringForUser. This was causing an issue where useENSNames wasn't doing looking for the viewer's ENS name
  2. Move optionality to type parameter. Now if you pass an array with no nulls/undefineds, Flow will understand that the result will have no nulls/undefineds

Keep stringForUser's advantageous property of only querying for ENS names if !isViewer

(This diff is ready for review now)

atul added inline comments.
lib/hooks/ens-cache.js
27–30 ↗(On Diff #21493)

I'd maybe pull cachedResult out of the return statement (similar to ethAddress) just since it's 4 lines here, but whatever you prefer.

53 ↗(On Diff #21493)

I'd maybe put the !ethAddress and fetchedAddresses.has(ethAddress) checks next to each other, but really doesn't matter.

75–79 ↗(On Diff #21493)

Might be missing something here, or just not know enough about Javascript, but is there a benefit to using a Map instead of a plain old Object as a key-value store?

This just seems a bit more verbose than for example:

setENSNames({
  ...ensNames,
  [ethAddress]: result
});
This revision is now accepted and ready to land.Jan 28 2023, 12:39 PM
lib/hooks/ens-cache.js
27–30 ↗(On Diff #21493)

Will do

53 ↗(On Diff #21493)

Will do

75–79 ↗(On Diff #21493)

I think Map can be slightly more performant, but I don't think it's too significant

This revision was landed with ongoing or failed builds.Jan 28 2023, 1:20 PM
This revision was automatically updated to reflect the committed changes.