-
Notifications
You must be signed in to change notification settings - Fork 158
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
fix: Refactor Farcaster typing to be explicit #37
Conversation
@@ -18,9 +19,9 @@ export function getHubClient(): HubRpcClient { | |||
* | |||
* @param body The JSON received by server on frame callback | |||
*/ | |||
async function getFrameValidatedMessage(body: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open question should we keep this as getFrameValidatedMessage
or have getFrameMessage
.
As now we are returing isValid
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should is data
more correct than message
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree and agree .. pr updated
src/core/farcasterTypes.ts
Outdated
}; | ||
} | ||
|
||
export interface FrameValidationResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be a tagged union, like:
type FrameValidationResponse =
| { isValid: true, data: FrameData }
| { isValid: false, data: undefined }
Then if isValid
is true the typechecker should know that data is defined and vice-versa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is pretty cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!!!
I am going to merge and then release a 0.2.0
version at 5pm PST.
With explicit call on this breaking changes.
What changed? Why?
2. Decided against importing the Farcaster types from @farcaster/hub-nodejs as the object types were more complex and could confuse consumers. Extract + simplify the interface.
Notes to reviewers
How has it been tested?