-
-
Notifications
You must be signed in to change notification settings - Fork 636
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: .extend performance #2328
fix: .extend performance #2328
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: fbcb2fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@actions/core@1.10.1, npm/@actions/github@5.1.1 |
@@ -209,9 +209,7 @@ export type ContractFunctionParameters< | |||
| allFunctionNames // show all options | |||
| (functionName extends allFunctionNames ? functionName : never) // infer value | |||
args?: (abi extends Abi ? UnionWiden<args> : never) | allArgs | undefined | |||
} & (readonly [] extends allArgs ? {} : { args: Widen<args> }) & | |||
// TODO: Remove `GetValue` from here (should be applied to top-level type as separate utility) | |||
GetValue<abi, functionName> |
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.
GetValue
was causing ContractFunctionParameters
to blow up when used with .extend
+ publicActions
### Description | ||
|
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.
no need for this lol
.github/workflows/verify.yml
Outdated
@@ -54,7 +54,7 @@ jobs: | |||
timeout-minutes: 5 | |||
strategy: | |||
matrix: | |||
typescript-version: ['5.3.2', 'latest'] | |||
version: ['5.0.4', '5.1.6', '5.2.2', '5.3.3', '5.4.5'] |
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.
test against more ts versions
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.
wonder if it's worth the work to fix the types that are incompatible with older versions
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.
indeed. on it!
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.
these type benchmarking tests make sure the types don't blow up too much
size-limit report 📦
|
export type WriteContractFunctionParameters< | ||
abi extends Abi | readonly unknown[] = Abi, | ||
mutability extends AbiStateMutability = AbiStateMutability, | ||
functionName extends ContractFunctionName< | ||
abi, | ||
mutability | ||
> = ContractFunctionName<abi, mutability>, | ||
args extends ContractFunctionArgs< | ||
abi, | ||
mutability, | ||
functionName | ||
> = ContractFunctionArgs<abi, mutability, functionName>, | ||
/// | ||
allFunctionNames = ContractFunctionName<abi, mutability>, | ||
allArgs = ContractFunctionArgs<abi, mutability, functionName>, | ||
// when `args` is inferred to `readonly []` ("inputs": []) or `never` (`abi` declared as `Abi` or not inferrable), allow `args` to be optional. | ||
// important that both branches return same structural type | ||
> = { | ||
address: Address | ||
abi: abi | ||
functionName: | ||
| allFunctionNames // show all options | ||
| (functionName extends allFunctionNames ? functionName : never) // infer value | ||
args?: (abi extends Abi ? UnionWiden<args> : never) | allArgs | undefined | ||
} & (readonly [] extends allArgs ? {} : { args: Widen<args> }) & | ||
GetValue<abi, functionName> |
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.
duplicated this type here for now. will address during some more extensive cleanup.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2328 +/- ##
=======================================
Coverage 99.67% 99.67%
=======================================
Files 673 673
Lines 57331 57367 +36
Branches 2770 2772 +2
=======================================
+ Hits 57145 57181 +36
+ Misses 169 168 -1
- Partials 17 18 +1 ☔ View full report in Codecov by Sentry. |
GetValue
was causingContractFunctionParameters
to blow up when used with.extend
+publicActions
.Added some type benchmarks using
attest
to protect against this in the future.Resolves #2322
PR-Codex overview
This PR focuses on improving performance and adding type checking.
Detailed summary
.extend
increateClient
writeContracts
function for better readability