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

fix(common): fix @endo/common integration breakage #1963

Merged
merged 2 commits into from
Jan 13, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Jan 13, 2024

closes: #XXXX
refs: #1935

Description

#1935 broke the agoric-sdk integration tests, i.e., would have broke integration with agoric-sdk . To repair the problem, this PR

  • gets rid of the index.js file which rolled up all the exports together
  • moves each of the source files -- each of which currently represents a separately exported utility -- to the top level.
  • changes the corresponding "exports": to use the same export name as the source file it exports, so that imports are the same whether processed by a tool that understands "exports": or not.
  • change all importers to do the deep import of the particular exports it needs.

Security Considerations

none

Scaling Considerations

By forcing importers to import only from the exporting file they need, some implementations (bundlers, packagers) can do tree-shaking, omitting code not reachable from imports. For @endo/common specifically, the savings are tiny. But the pattern could bring substantial savings elsewhere.

Documentation Considerations

nothing interesting

Testing Considerations

none

Upgrade Considerations

The old exports prior to #1935 that were exporting some of these, for compat, still reexports these. But it deprecates those reexports, with advice that importers do a deep import directly from @endo/common/.... Thus, this PR should not have any compat issues compared to the state preceding #1935.

This PR is technically a compat break with #1935 itself. But #1935 was just merged and never released, so that is not a source of worry.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

That covers it. It might or might not save time to propose a no-op PR to agoric-sdk pinned to this branch to verify that CI settles.

packages/common/package.json Outdated Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Jan 13, 2024

That covers it. It might or might not save time to propose a no-op PR to agoric-sdk pinned to this branch to verify that CI settles.

Agoric/agoric-sdk#7937

@erights
Copy link
Contributor Author

erights commented Jan 13, 2024

For the record, Agoric/agoric-sdk#7937 CI is currently green with #endo-branch: markm-fix-common-package, so merging. Afterwards, I'll switch Agoric/agoric-sdk#7937 back to master.

@erights erights merged commit 73b5059 into master Jan 13, 2024
14 checks passed
@erights erights deleted the markm-fix-common-package branch January 13, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants