This method will allow us to query the cache for ENS avatars.
Depends on D7040
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Some questions inline
| lib/utils/ens-cache.js | ||
|---|---|---|
| 243–250 ↗ | (On Diff #23635) | Is there a reason we can't do something like this? |
| 265–269 ↗ | (On Diff #23635) | It looks like on line 282 we're checking if avatarURI is null but not whether it's undefined. Are we trying to encode two different states using null vs undefined? Would prefer if we could return something that indicates the result of fetchENSAvatarPromise more explicitly. Maybe an object with a field like indicates the reason for a missing avatar? Something loosely along the lines of: export type FetchENSAvatarPromiseRetVal = | { +avatar: string, } | { +avatar: null, +reason: 'timeout' | 'not_set' }; Alternatively, could rethrow from within the catch block and then wrap the await fetchENSAvatarPromise on line 280 in a try/catch and handle the timeout case there accordingly? Personally would prefer something closer to the second approach. |
| 274 ↗ | (On Diff #23635) | Might be missing something here, but it seems redundant to include normalizedETHAddress in ENSAvatarQueryCacheEntry when normalizedETHAddress is the key for this.avatarQueryCache? |
| lib/utils/ens-cache.js | ||
|---|---|---|
| 243–250 ↗ | (On Diff #23635) | Yes, it makes it impossible to dedup promises. async functions always create new promises |
| 265–269 ↗ | (On Diff #23635) | I think null/undefined is fine when it's within a local function scope like this. I think the boilerplate you're proposing is only clearly worth it when value is crossing file bounds, or at least top-level function bounds |
| 274 ↗ | (On Diff #23635) | I think this is generally good practice actually... the effect on memory is negligible and it makes it easier to reason about a "cache entry" on its own |
| lib/utils/ens-cache.js | ||
|---|---|---|
| 265–269 ↗ | (On Diff #23635) | That's fair. Just generally find that distinguishing between null/undefined to encode different states makes code tricker to reason about... which in turn could increase the risk for bugs if/when someone modifies this code in the future. I think about it similarly to when ?boolean is used to encode three states instead of two... there's usually a clearer and more explicit way to design things |
| 274 ↗ | (On Diff #23635) | Yeah actually on second thought I take that back. I think it makes sense for the ENSAvatarQueryCacheEntry object to be able to "stand alone." |