refactor(common): cleanup some inferred types, add TS benchmark #2956
Conversation
|
| }) as unknown as GetContractReturnType<TAbi, { public: TPublicClient; wallet: TWalletClient }, TAddress> & { | ||
| write: unknown; | ||
| }; | ||
| }) as never; |
There was a problem hiding this comment.
is there any practical benefit to this approach?
iirc we copied the as unknown as ... pattern from viem, so I wonder if there's some improvements to be done there too
There was a problem hiding this comment.
Yes, a few things:
- Avoid duplicating long, complex expressions with no additional type safety
- Avoid comparing potentially complex expressions that don't need to be compared because you're casting them anyways. This comparison shouldn't occur in the
as unknown as Xcase but it would occur if e.g. you have an explicit return type and also case the return value in some areas to another complex expression instead of justneverwhen the only goal is just to allow that return anyways.
There was a problem hiding this comment.
happy with this change, but just FYI, I reverted this back to what's in main and I see no change in instantiations
There was a problem hiding this comment.
Yeah I should have been clearer, I made this change while optimizing perf based on the traces I saw but it just led me to the variance issue in viem. I know there is no perfect impact here, and specifically casting as unknown as X for a function with an explicit return type of X will not cost anything.
The problem is if X is a long expression, not only is it unnecessary duplication, if the two ever get slightly out of sync, e.g. an extra optional parameter is added to the return but not the cast, then TS does have to compare the instantiations which can be very expensive.
| @@ -37,6 +37,7 @@ | |||
| "package.json": "pnpm sort-package-json" | |||
| }, | |||
| "devDependencies": { | |||
| "@ark/attest": "0.9.2", | |||
There was a problem hiding this comment.
I saw you did this in another PR and I had moved it out of the workspace root in a follow up PR to be consistent with how we've historically managed deps. Open to changing this approach though and curious to hear your thoughts - should common test deps be added to the workspace root instead of individual packages?
There was a problem hiding this comment.
Yes, devDependencies should generally be managed at the monorepo root in my opinion. It prevents having to manage multiple versions and continue to install them in new packages as needed, and there's really no downside as long as you're not importing them in source. I have an eslint rule that warns if I'm importing from a package not listed in dependencies.
There was a problem hiding this comment.
cool, added a follow up here: #2962
| }); | ||
|
|
||
| return t; | ||
| }).types([20704, "instantiations"]); |
There was a problem hiding this comment.
I saw this 2x when using TS types from source as opposed to DTS. I wonder if we should have benchmarks for both cases?
There was a problem hiding this comment.
Yeah I saw this as well. The differences usually aren't so drastic for me but it makes sense in a repo like this where you have a lot of complex function returns etc, because it's checking your own .ts code as well.
The .d.ts test is going to be the more relevant for your users, but including paths would still give you something to iterate on so I think it could be reasonable to include both.
holic
changed the title
First of all, from what I can tell the last round of changes have had a huge positive improvement.
I haven't found any low-hanging fruit for reducing instantiation count so far other, but made some marginal type improvements along the way.
More importantly, after some iteration analyzing traces, I found a single change that could make a massive difference in check time despite not affecting instantiations. There's a type called
SimulateContractReturnTypein Viem that accepts 8 parameters and apparently has a complex enough stucture that it takes TS almost 2 seconds just to determine each parameter's variance.With Variance Checks
However, with explicit annotations, these checks can be avoided altogether for external consumers. I added the annotations to viem's
.d.tsinnode_modulesand saw a significant improvement in check time:Now, checking the entire
setupNetwork.tsfile takes significantly less time (1.2s) than just those variance checks.Unfortunately this isn't actionable in this PR, but I think it would make sense to follow up with a PR to Viem to add these annotations. The inferred annotations from TS can be seen in the first image above, although unfortunately independent (bivariant) can't be annotated directly.