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: wrap hub-web client to provide consistent APIs with hub-nodejs #811

Merged
merged 7 commits into from
Apr 4, 2023

Conversation

PangZhi
Copy link
Contributor

@PangZhi PangZhi commented Apr 4, 2023

Motivation

As part #573, wrap client of hub-web so APIs are consistent with hub-nodejs. This will provide better developer experience.

Change Summary

  • wrap client of hub-web response Promise -> Promise<HubResult>
  • also fixed the camelCase issue for generated ts

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review

  • PR title adheres to the conventional commits standard
  • PR has a changeset
  • PR has been tagged with a change label(s) (i.e. documentation, feature, bugfix, or chore)
  • PR does not require changes to the protocol

Additional Context

@PangZhi PangZhi added the t-feat Add a new feature or protocol improvement label Apr 4, 2023
@changeset-bot
Copy link

changeset-bot bot commented Apr 4, 2023

🦋 Changeset detected

Latest commit: e95e1a6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@farcaster/hub-web Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@PangZhi PangZhi requested a review from pfletcherhill April 4, 2023 19:20
Copy link
Contributor

@pfletcherhill pfletcherhill left a comment

Choose a reason for hiding this comment

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

One question about the client exported methods, otherwise looks good!


export type RpcWebClient = WrappedClient<HubService>;

export const getRpcWebClient = (url: string, isBrowser = true): WrappedClient<HubService> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we export getSSLHubRpcClient and getInsecureHubRpcClient to be consistent with hub-nodejs?

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Patch coverage: 85.84% and project coverage change: -0.45 ⚠️

Comparison is base (71d6494) 73.03% compared to head (e95e1a6) 72.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #811      +/-   ##
==========================================
- Coverage   73.03%   72.59%   -0.45%     
==========================================
  Files          61       63       +2     
  Lines        5359     5429      +70     
  Branches     1216     1220       +4     
==========================================
+ Hits         3914     3941      +27     
- Misses       1351     1386      +35     
- Partials       94      102       +8     
Impacted Files Coverage Δ
apps/hubble/src/eth/utils.ts 100.00% <ø> (ø)
apps/hubble/src/hubble.ts 1.84% <ø> (-0.03%) ⬇️
apps/hubble/src/network/p2p/protocol.ts 100.00% <ø> (ø)
apps/hubble/src/rpc/adminServer.ts 1.88% <0.00%> (ø)
apps/hubble/src/rpc/server.ts 78.78% <ø> (-0.46%) ⬇️
apps/hubble/src/storage/db/hubState.ts 36.36% <ø> (ø)
apps/hubble/src/storage/db/idRegistryEvent.ts 83.87% <ø> (ø)
apps/hubble/src/storage/db/nameRegistryEvent.ts 100.00% <ø> (ø)
apps/hubble/src/storage/db/rocksdb.ts 90.56% <ø> (+0.08%) ⬆️
apps/hubble/src/storage/engine/seed.ts 100.00% <ø> (ø)
... and 27 more

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@PangZhi PangZhi merged commit ce7929e into farcasterxyz:main Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-feat Add a new feature or protocol improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants