-
Notifications
You must be signed in to change notification settings - Fork 214
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
refactor: postponed removals and simplifications #8771
Conversation
3ec1016
to
1b5fbf7
Compare
@turadg , at https://github.com/Agoric/agoric-sdk/actions/runs/7591791789/job/20680380230?pr=8771#step:4:361 I'm getting one block of type errors I'd like your help on. These errors only happen indirectly in the @agoric/casting package. There are no such errors in the packages these things are imported from. Nor in any other packages that import them. What's special about @agoric/casting? |
At endojs/endo#1933 (comment) , I ask about possible interaction with some endo side changes that were also waiting until after the endo-agoric-sdk sync. |
Nothing particularly special. Each package is a separate TypeScript project so they each resolve differently (see #4404 as a spike to unify). In this case We rely on searching because of the decision years ago to rely on ambient types. The more we use type modules (with imports and exports) the less we'll have this problem. One reason it's happening in your PR is that making The change also reduces the maxNodeModuleJsDepth to 1, shrinking the type import graph. That caused another problem, with ThisType in Separately, your "cleanups in this PR" list has several items but so far it's all in one commit. Please consider separating the changes by commit. It will be easier to review that way. |
6e0290e
to
ac7afd2
Compare
The vast majority of the bulk were changing imports, and use of the |
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.
This looks fine to me.
My impression of the current intended process is that we can merge changes to master as they get approved, and the hard version compatibility questions will be answered as we decide when to migrate to a release branch. @warner, is that a correct reading of the theory?
a49db10
to
4a722f8
Compare
ac7afd2
to
1c8af6d
Compare
f4b29d2
to
b5120d8
Compare
b5120d8
to
d59dc41
Compare
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
Staged on #8781
closes: #XXXX
refs: #8749 endojs/endo#1985 #8781
Description
Now that a recent endo is released and agoric-sdk has been upgraded to depend on it, we can finally get to a lot of agoric-sdk technical-debt cleanups that we've postponed. The cleanups in this PR include:
@agoric/same-structure
package, that had no remaining users anyway.Security Considerations
Less code helps security. No other effect on security
Scaling Considerations
none. (or minor help due to smaller total code)
Documentation Considerations
Once these reductions are complete and we can delete legacy exporters, there will be less to document.
Testing Considerations
Of the redundant code copies here, the tests here a bit bigger than the ones in their new home. So endojs/endo#1985 will preserve this extra test code.
Upgrade Considerations
@ivanlei added you as a reviewer for advice on when it would not be too disruptive to merge this.