Skip to content
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.

Sign up for GitHub

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

Jump to bottom

End runtime imports of types modules #6512

Open
turadg opened this issue Oct 31, 2022 · 6 comments · Fixed by #9076
Open

End runtime imports of types modules #6512

turadg opened this issue Oct 31, 2022 · 6 comments · Fixed by #9076
Assignees
@turadg
Labels
hygiene Tidying up around the house needs-design technical-debt vaults_triage DO NOT USE

Comments

@turadg
Copy link
Member

turadg commented Oct 31, 2022 •
edited
Loading

What is the Problem Being Solved?

To use ambient types from one package in another, we've had to resort to runtime imports of empty modules (containing only types). For example,

agoric-sdk/packages/agoric-cli/src/init.js

Lines 4 to 7 in 8980a2d

// Ambient types. Needed only for dev but this does a runtime import.
import '@endo/captp/src/types.js';
import '@agoric/swingset-vat/exported.js';

Description of the Design

Eliminate runtime imports that are just for types.

Most of these cases will be by ensuring the necessary types are exported from the root of the package so that they can be imported where used in a concise way. E.g. import('@endo/captp').TrapImpl

Remove or drop to devDependencies any packages that are currently dependencies in package.json just because of these imports.

Security Considerations

--

Test Plan

Ensure that all the package dependencies are for modules needed at runtime.
@turadg turadg added hygiene Tidying up around the house technical-debt labels Oct 31, 2022
@turadg turadg mentioned this issue Oct 31, 2022
Merged
@mhofman
Copy link
Member

mhofman commented Nov 1, 2022 •
edited
Loading

This may require moving back to dev / removing some packages that are currently dependencies in package.json just because of these imports.

@turadg
Copy link
Member Author

turadg commented Nov 1, 2022

This may require moving back to dev / removing some packages that are currently dependencies in package.json just because of these imports.

Good to specify. I've added it to the body.

@ivanlei ivanlei added the vaults_triage DO NOT USE label Jan 4, 2023
@dckc
Copy link
Member

dckc commented May 18, 2023

just spent 20min puzzling over a case of this; had to phone a friend, so multiply some of that by 2

@dckc dckc mentioned this issue May 18, 2023
Merged
@turadg
Copy link
Member Author

turadg commented Sep 23, 2023

For prioritization: the ambient imports are adding ~60kb to each bundle of core proposals. Those cost money to put on chain.

@turadg turadg mentioned this issue Sep 28, 2023
Merged
@turadg turadg self-assigned this Oct 10, 2023
@dckc dckc mentioned this issue Dec 30, 2023
Closed
2 tasks
@dckc
Copy link
Member

dckc commented Mar 12, 2024

While working on Agoric/dapp-agoric-basics#21 , I learned that it's not necessary to import the ambient types into the executable module. A types.js file in the same directory that imports them seems to suffice.

https://github.com/Agoric/dapp-agoric-basics/blob/e9f8604fd875f33a58263758d1a3e1184703de40/contract/src/platform-goals/types.js

@turadg turadg mentioned this issue Mar 12, 2024
Merged
@mergify mergify bot closed this as completed in #9076 Mar 12, 2024
@turadg
Copy link
Member Author

turadg commented Mar 12, 2024

Reopening because #9076 only handled agoric/.*types but missed agoric/.*exported

@turadg turadg reopened this Mar 12, 2024
@dckc dckc mentioned this issue Apr 2, 2024
Merged
3 tasks
@turadg turadg mentioned this issue Apr 29, 2024
Merged
mergify bot added a commit that referenced this issue May 1, 2024
@mergify
explicit type exports for agoric/notifier (#9305)
e456308 
refs: #6512

## Description

Progress on #6512, spawned from #9300.

This gives the @agoric/notifier package explicit exports.

Many downstream packages were relying on @agoric/notifier ambients for
types from @agoric/internal or Endo, so this also adds an `exported.js`
to @agoric/internal to provide those. (E.g. `ERef`).

### Security Considerations
none, types
<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations
none, types
<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

might improve API docs of Notifier package
<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations
CI
<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations
none, types
<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
@turadg turadg mentioned this issue May 17, 2024
Merged
mergify bot added a commit that referenced this issue May 20, 2024
@mergify
reduce ambient imports and drop maxNodeModuleJsDepth (#9373)
980ff41 
refs: #6512

## Description

- further improves recent TypeScript best practices doc
- turns the zoe/exported.js into a .d.ts so it can be used in
triple-slash references
- adopts that and stops the runtime imports of zoe/exported (progress on
#6512)
- remove the `maxNodeModuleJsDepth` now that the types resolve without
- burns down imports so there are no inter-package runtime imports of
typedefs. (leaving just a bit for #6512)

I put commit checkpoints of type coverage to guard against any
regression, though I took them out again when merging


### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@turadg turadg

Labels
hygiene Tidying up around the house needs-design technical-debt vaults_triage DO NOT USE
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

end runtime imports of other packages for types Agoric/agoric-sdk
5 participants
@turadg @mhofman @dckc @Tartuffo @ivanlei