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.

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

chore: avoid some unnecessary bundling in vaultFactory etc. #7169

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

dckc
Copy link
Member

@dckc dckc commented Mar 15, 2023

refs: #6687

Description

  • Avoid bundling the binaryVoteCounter.js contract in bundle-vaultFactory.js
  • Use static type check and unit tests rather than runtime import to avoid bundling all of ERTP just to import { Stable, Staking } from 'tokens.js';.

Discovered while auditing bootstrap contents (#6687, #7049).

Scaling Considerations

slightly smaller bundle size:

bundle before after savings
bundle-VaultFactory.js 1492220 1476536 1%

Testing Considerations

Moved assertKeywordName(Stable.symbol) and the like from a runtime check to a unit test.

@datadog-full-agoric
Copy link

datadog-full-agoric bot commented Mar 15, 2023

Datadog Report

Branch report: dc-token-slim
Commit report: 7d9098d

agoric-sdk: 0 Failed, 0 New Flaky, 3418 Passed, 57 Skipped, 13m 57.2s Wall Time

@dckc dckc added the bypass:integration Prevent integration tests from running on PR label Mar 15, 2023
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

*
* @type {typeof import('@agoric/ertp').AssetKind.NAT}
*/
const NAT = 'nat';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move AssetKind to agoric/internal to not duplicate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep wondering about something like @agoric/ertp/constants, but I'm not inclined to mess with the ERTP package API just now.

In the Java world, I gather it's conventional to provide a -api.jar separate from the main implementation .jar file for things like this. I keep wondering about analogs in the JS world... but I guess the JS norm is to just leave it to tree-shaking tools..

@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Mar 15, 2023
Use static type check and unit tests rather than runtime import to
avoid bundling all of ERTP just to get, for example, Stable.symbol.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants