Skip to content
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

feat: cast actions deeplink v2 #251

Merged
merged 9 commits into from Apr 28, 2024

Conversation

dalechyn
Copy link
Collaborator

@dalechyn dalechyn commented Apr 12, 2024

I've set changesets to a minor bump as it is gonna break the API of Button.AddCastAction and is gonna require a user to pass the options as the third argument of the .castAction handler.

While being a breaking change, it is pretty easy to migrate it.

Copy link

vercel bot commented Apr 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
frog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 26, 2024 0:39am
frog-frame ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 26, 2024 0:39am

Copy link

vercel bot commented Apr 12, 2024

@dalechyn is attempting to deploy a commit to the wevm Team on Vercel.

A member of the Team first needs to authorize it.

src/frog-base.tsx Outdated Show resolved Hide resolved
>
}
> = /* We enforce `options` parameter to not be partial for Cast Actions*/
M extends 'cast-action'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I would rather avoid this ternary. Can we instead make options optional within the parameter definition itself if the method extends "cast-action"?

(
  path: P,
  handler: H<...>,
  ...rest: (M extends 'cast-action' ? [options: RouteOptions<M>] : [options?: RouteOptions<M>])
): FrogBase<...>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brilliant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

export type RouteOptions = Pick<FrogConstructorParameters, 'verify'> & {
fonts?: ImageOptions['fonts'] | (() => Promise<ImageOptions['fonts']>)
}
export type RouteOptions<M extends string = string> = Pick<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type RouteOptions<M extends string = string> = Pick<
export type RouteOptions<method extends string = string> = Pick<

We can conform to our convention of generics as we aren't working on the Hono type fork (types/routes.ts) anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

? {
fonts?: ImageOptions['fonts'] | (() => Promise<ImageOptions['fonts']>)
}
: M extends 'cast-action'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: M extends 'cast-action'
: M extends 'castAction'

Would be good if it mapped to the casing of the method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@@ -17,5 +17,5 @@ export function getRouteParameters<env extends Env, handler>(
else middlewares.push(parameters[i])
}

return [parameters[0], middlewares, handler!, options ?? {}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this diff intended? Previous implementation is correct, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was intended, just fixed it with a more narrowed typing.

@dalechyn
Copy link
Collaborator Author

need to correct the docs here

@jxom jxom merged commit f841edc into wevm:main Apr 28, 2024
7 of 9 checks passed
@github-actions github-actions bot mentioned this pull request Apr 28, 2024
dalechyn added a commit that referenced this pull request May 2, 2024
* feat: cast actions deeplink v2

* chore: changesets

* docs: update

* chore: update changesets

* nit: drop console.log

* refactor: apply changes from code review

* docs: fix

* docs: nit

* docs: fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D: Medium Difficulty: Medium P: Medium Priority: Medium T: Feature Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants