-
Notifications
You must be signed in to change notification settings - Fork 128
Refactored SDK package to TypeScript #2352
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
Conversation
🦋 Changeset detectedLatest commit: ba6de5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
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.
Here's the comments I had so far (I've only got through 100/131 files so far)
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.
Pull Request Overview
This PR refactors the SDK package to TypeScript, significantly increasing TS/JS and type coverage ratios. Key changes include removing legacy JavaScript files, introducing new TypeScript versions of builder functions, and updating related test files.
Reviewed Changes
Copilot reviewed 130 out of 131 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/sdk/src/build/build-authorizations.js | Removed legacy JS authorization builders |
packages/sdk/src/build/build-at-latest-block.ts | New TS builder for fetching the latest block |
packages/sdk/src/build/build-at-block-id.ts | Updated TS implementation with explicit typing |
packages/sdk/src/build/build-at-block-height.ts | New TS builder for fetching a block by height |
packages/sdk/src/block/block.ts | Updated TS block fetching logic |
packages/sdk/src/account/account.ts | Updated account fetch with TS imports and improved typing |
packages/sdk/src/VERSION.ts | New TS version file export |
packages/kit/src/mocks/fcl.ts | Updated mock to use type assertion for actualFcl |
.changeset/tame-pianos-doubt.md | Changeset reflecting the refactoring |
Files not reviewed (1)
- packages/sdk/package.json: Language not supported
acc[v.name] = await recurseDecode(v.value, decoders, [...stack, v.name]) | ||
return acc | ||
}, Promise.resolve({})) | ||
const decodeDictionary = async ( |
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.
Do we want JSDoc for all these?
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.
If the function is exported, I think we should add JSDoc comments. Otherwise, if it’s a simple internal function, we could skip it for now. That said, we can always add them later, and the end, JSDoc comments are safe to include in separate PRs since they only affect documentation and not the actual code.
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.
It looks great, I just ran through most of the files now.
There's a couple types that I'm not sure about and I'll have to dive a bit deeper into the code for tomorrow to double check (specifically some things with the authz functions and decoding). But I'll get on this ASAP
|
||
type AcctFn = (acct: InteractionAccount) => InteractionAccount | ||
type AccountFn = AcctFn & Partial<InteractionAccount> | ||
export type AcctFn = (acct: InteractionAccount) => InteractionAccount |
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.
export type AcctFn = (acct: InteractionAccount) => InteractionAccount | |
export type AuthorizationFn = (acct: InteractionAccount) => InteractionAccount |
For consistency with docs https://developers.flow.com/tools/wallet-provider-spec/authorization-function#how-to-create-an-authorization-function
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.
👍
export type AccountFn = | ||
| (AcctFn & Partial<InteractionAccount>) | ||
| Partial<InteractionAccount> |
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.
Can we name this something without the Fn suffix? IMO it's probably a bit of a misnomer since it's a union type.
How about something like AccountAuthorization
?
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.
Yep AccountAuthorization fits really well, considering also the refactoring that AcctFn is now AuthorizationFn 👍
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.
Two naming comments, but other than that it looks good to me 🥳
# Conflicts: # package-lock.json # packages/kit/src/__mocks__/fcl.ts
* PKG -- [transport-http] Create SubscriptionManager and subscribe function (#2019) * PKG -- [sdk] Create SDK subscribe function (#2024) * Update to latest streaming API changes (#2202) * PKG -- [transport-http] Add Blocks DataProvider (#2033) * PKG -- [transport-http] Add BlockDigests DataProvider (#2034) * PKG -- [transport-http] Add BlockHeaders DataProvider (#2035) * PKG -- [transport-http] Add TransactionStatuses DataProvider (#2041) * PKG -- [transport-http] Add Events DataProvider (#2037) * PKG -- [fcl] Expose new streaming endpoints (#2038) * PKG -- [transport-http] Add AccountStatuses DataProvider (#2036) * Switch to synchronous subscriptions with callback-based error reporting (#2242) * Switch `fcl.events` to use new event streaming method (#2241) * Fix resume args for `events` & `account_statuses` (#2249) * Cleanup subscriptions types & align with decoded representation (#2256) * Fix endpoint for WS subscriptions (#2277) * Replace `fcl.tx` with realtime streaming (#2258) * Reorganize subscriptions types (#2301) * Add legacy `fcl.events` fallback for emulator (#2287) * Add emulator fallback for fcl.tx (#2294) * Fix resume args type check not checking if value is falsy (#2309) * Add missing decoders (#2320) * Improve WebSocket Lifecycle Management (#2312) * Add changesets * Fix Account Status Decoding * Add `useIsCadenceWalletConnected` hook (#2326) * Expose origin in `onMessageFromFCL` callback (#2384) * Update and clean up WalletConnect dependencies (#2382) * Deprecate manual `appIdentifier` in favour of window origin (#2378) * Updated types to interfaces and comments cleanup (#2393) * Updated types to interfaces and comments cleanup * Added changeset --------- Co-authored-by: mfbz <[email protected]> * Version Packages (#2371) * Add `useFlowChainId` hook (#2367) * Rename `useFlowTransaction` to `useFlowTransactionStatus` (#2413) * Rename tx hook to tx status hook * Fix import --------- Co-authored-by: Chase Fleming <[email protected]> * Add changeset (#2414) * Add changeset * Update moody-ligers-rule.md * Update .changeset/moody-ligers-rule.md Co-authored-by: Jordan Ribbink <[email protected]> --------- Co-authored-by: Chase Fleming <[email protected]> Co-authored-by: Jordan Ribbink <[email protected]> * Add comment about renaming (#2416) Co-authored-by: Chase Fleming <[email protected]> * Fix `useFlowRevertibleRandom` range (#2417) * Update readme with `useFlowRevertibleRandom` hook (#2419) Co-authored-by: Chase Fleming <[email protected]> * Add `useCrossVmBatchTransaction` function (#2368) * Refactored SDK package to TypeScript (#2352) * Added onflow/typedefs and onflow/types types into sdk package * Updated first part of files * Updated encode and decode folders * Updated send.js * Updated sdk resolve files * Fixed build problems * Updated wallet-utils folder * Moved typedefs and types back to their package * Fixed build * Refactored more js to ts files * Fixed tests * Added onflow/types re-export from sdk * Added missing onflow/types package to sdk * Updated readme with typescript examples * Fixed build error on type problem * Added changeset file * Cleaned up types and docs * Minor comments fix * Removed unused cast * Added suport to conditional interaction callbacks in build * After install update * Fixed build-transaction input params * Updated send function null check * Re-added resolve comments to two special functions * Added Voucher export from sdk * Improved input params for send function * Renamed InteractionCallback to InteractionBuilderFn * Improve with central function definition from interaction file * Improved atBlockId validator types * Strongly typed voucher intercept param * Improved signing function interface and minor refactoring * Refactored AcctFn function name * Refactored AccountFn name --------- Co-authored-by: mfbz <[email protected]> * Fix payload type * Fix payload decoding * Fix Typos in Documentation Files (#2420) * Enter alpha mode (#2421) * Version Packages (alpha) (#2396) * Fix current user returning false before checking authenticated (#2428) Co-authored-by: Chase Fleming <[email protected]> * Allow undefined value for `useFlowTransactionStatus` (#2439) * Fixed fcl.mutate hanging (#2434) * Fixed fcl-mutate not working * Update packages/sdk/src/block/block.ts Co-authored-by: Jordan Ribbink <[email protected]> * Update packages/sdk/src/block/block.ts Co-authored-by: Jordan Ribbink <[email protected]> * Update packages/sdk/src/block/block.ts Co-authored-by: Jordan Ribbink <[email protected]> * Update packages/sdk/src/node-version-info/node-version-info.ts Co-authored-by: Jordan Ribbink <[email protected]> * Update packages/sdk/src/resolve/resolve.ts Co-authored-by: Jordan Ribbink <[email protected]> * Added validator improvement * Create four-frogs-jump.md --------- Co-authored-by: mfbz <[email protected]> Co-authored-by: Jordan Ribbink <[email protected]> * Version Packages (alpha) (#2440) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Fix build * Prettier --------- Co-authored-by: Jordan Ribbink <[email protected]> Co-authored-by: Jordan Ribbink <[email protected]> Co-authored-by: Michael Fabozzi <[email protected]> Co-authored-by: mfbz <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Chase Fleming <[email protected]> Co-authored-by: leopardracer <[email protected]>
Closes #2332.
Migration TS/JS ratio
From (20 / 123) 16.26% → To (106 / 125) 84.80% (Improvement ~70%)
Type Coverage
From (8605 / 9982) 86.18% → To (9669 / 10520) 92.21% (Improvement ~6%)