-
Notifications
You must be signed in to change notification settings - Fork 212
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
fewer runtime imports for types #8410
Conversation
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.
OMG amazing! I had tried this locally but ran into a bunch of side-effects where it could no longer find some declarations. If CI passes, I think I'm good getting this merged.
NB: I will always have a fear with changes to ambient types that CI will miss something because we don't have noImplicitAny
enabled. I have no idea how to assuage that fear.
2d163f5
to
feecbe0
Compare
Me too. I added some tooling to report code coverage. It didn't go down in any packages, and went up slightly in agoric-cli. I considered adding it to CI but I'm not ready to support whatever kinks that introduces. For now it helps to run on-demand. Current coverage: (massaged from
|
@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours |
1 similar comment
@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours |
The postpack of a previous package only cleans up files in that package. Its prepack may build files in other packages that it doesn't clean up. The remants were causing errors like: ``` > @agoric/governance@0.10.4-dev-5988fad.0 prepack /home/runner/work/agoric-sdk/agoric-sdk/packages/governance > echo "export {}; " | cat - src/types-ambient.js > src/types.js && tsc --build tsconfig.build.json error TS5055: Cannot write file '/home/runner/work/agoric-sdk/agoric-sdk/packages/ERTP/exported.d.ts' because it would overwrite input file. ``` (in 'https://github.com/Agoric/agoric-sdk/actions/runs/6346051963/job/17239010386?pr=8410'\) This makes the build clean up files in the way.
feecbe0
to
dbb6522
Compare
refs: #6512
Description
To help with #6512 @mhofman suggested triple-slash directives.
That does the trick for a lot of the cases, which are included here. The ones left I had some trouble with and I'll leave for another PR.
Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
We could say that devs should use the triple-slash instead of import for types imports, but it doesn't yet work reliably. So this is just some progress.
Testing Considerations
CI
Upgrade Considerations
Could possibly affect consumers of these packages but can't affect runtime.