Page MenuHomePhabricator

[lib] include embeds in neynar client's postCast method
ClosedPublic

Authored by will on Fri, Dec 6, 8:26 PM.

Details

Summary

This adds embeds as a parameter to the postCast method for the neynar client and makes text optional

Depends on D14086

Test Plan

Tested later in the stack. Was able to successfully post the frame url to warpcast

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Dec 6, 8:45 PM
Harbormaster failed remote builds in B32959: Diff 46253!
will requested review of this revision.Fri, Dec 6, 9:17 PM
keyserver/src/responders/farcaster-webhook-responders.js
326–331 ↗(On Diff #46256)

Unsure if including undefined as an argument here is fine. The text parameter for postCast is optional. I found another example of us doing this in a call to persistCryptoStore in web/shared-worker/worker/worker-crypto.js:

await persistCryptoStore(undefined, true);
ashoat added inline comments.
lib/utils/neynar-client.js
349–352 ↗(On Diff #46256)

Can we turn this into a "bag of params" instead? (Basically have it take one param, which is an object that can contain four properties)

async postCast(
  params: {
    +signerUUID: string,
    +parent: string,
    +embeds?: ?$ReadOnlyArray<{ +url: string }>,
    +text?: ?string,
  }
): Promise<NeynarPostCastResponse> {
351 ↗(On Diff #46256)

You're missing a +

This revision is now accepted and ready to land.Sun, Dec 8, 8:10 PM
keyserver/src/responders/farcaster-webhook-responders.js
326–331 ↗(On Diff #46256)

This will get addressed by the bag of params approach outlined below. Including in next rebase

lib/utils/neynar-client.js
349–352 ↗(On Diff #46256)

Yeah. Unsure if we have a strict coding standard but it sounds like whenever we have four or more parameters for a function (or optional parameters), we should consider using a bag of params. Makes it easier to read function calls

I'll include this in the next rebase

lib/utils/neynar-client.js
349–352 ↗(On Diff #46256)

No strict rules, but 4 is a good guideline

will edited the summary of this revision. (Show Details)

rebase