fix tsc errors detected by skipLibCheck: false #9457
Conversation
the one pulled in by `@rollup/pluginutils` was missing exports
Deploying agoric-sdk with
|
| Latest commit: |
0701b91
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d24020a3.agoric-sdk.pages.dev |
| Branch Preview URL: | https://ta-ts-libcheck.agoric-sdk.pages.dev |
| @@ -39,6 +39,7 @@ | |||
| }, | |||
| "resolutions": { | |||
| "**/protobufjs": "^7.2.6", | |||
| "**/@types/estree": "^1.0.0", | |||
There was a problem hiding this comment.
seems like I should understand this better. I'm not going to hold up this PR over it, though.
cc @kriskowal
There was a problem hiding this comment.
some rollup package depends on an earlier version of @types/estree that has errors
| @@ -331,11 +331,14 @@ export const makeBridgeManager = async ({ | |||
| const bridgeManager = E(vat).provideManagerForBridge(bridge); | |||
| bridgeManagerP.resolve(bridgeManager); | |||
| provisionBridgeManager.resolve( | |||
| // @ts-expect-error XXX EProxy | |||
There was a problem hiding this comment.
😕
I suppose I should check out the branch and look at the whole diagnostic.
If you have it handy, please share.
There was a problem hiding this comment.
the BridgeId parameter is getting lost, I think by the EProxy
| StartFns extends Record<WellKnownNames['installation'], ContractStartFn>, | ||
| StartFns extends Record<WellKnownName['installation'], ContractStartFn>, |
There was a problem hiding this comment.
!!
I wonder how long that bug was lying around.
| @@ -104,13 +104,16 @@ export type StartInstance = <SF>( | |||
| issuerKeywordRecord?: Record<Keyword, Issuer<any>>, | |||
| // 'brands' and 'issuers' need not be passed in; Zoe provides them as StandardTerms | |||
| terms?: Omit<StartParams<SF>['terms'], 'brands' | 'issuers'>, | |||
| // @ts-expect-error XXX | |||
There was a problem hiding this comment.
hm. The other @ts-expect-error in this PR seem to have local impact, but this one is imported in various places.
Seems like I should look into this more closely, but I'm not sure it's the best use of time.
in case anyone else finds a moment to take a look at this PR: cc @mhofman @gibson042
There was a problem hiding this comment.
to be clear this is maintaining earlier behavior, since the .d.ts isn't checked. It just makes it so that when someone does skipLibCheck: false this won't register as an error. It also gives a clue to someone working in this file that there's a problem here
Description
In a recent discussion we noticed that the test wasn't showing a type error where it should have: #9452 (comment)
It would have with
noImplicitAny: falsebut that pulls in way too many other errors. While investigating I noticed some other things that could be cleaned up and tackled those here.The remaining blocker for
skipLibCheck: falseis the Telescope output indist. The .js files have@ts-nocheckbut the .d.ts don't. Surprising but not worth more effort.Security Considerations
n/a, types
Scaling Considerations
n/a, types
Documentation Considerations
none
Testing Considerations
CI
Upgrade Considerations
Potential cherry-pick complexities but we're moving to release based on master