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

refactor: start replacing Far with makeExo #9046

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

erights
Copy link
Member

@erights erights commented Mar 7, 2024

closes: #XXXX
refs: endojs/endo#2118

Description

See endojs/endo#2118 for the description. Pretty much all said there wrt endo applies here wrt agoric-sdk.

However, this first commit is only of the automated part. Due to the greater surface area, none of the manual steps are started, and so massive CI failures are expected.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@erights erights requested review from dtribble and ivanlei March 7, 2024 04:21
@erights erights self-assigned this Mar 7, 2024
@@ -8,7 +8,7 @@ harden(defaultLabelToKeys);
/**
* @param {string} debugName Only used internally for diagnostics, not available to user code
* @param {import('./types.js').Stores} stores
* @param {import('@agoric/swingset-liveslots').MapStore<string, any>} [backingStore]
* @param {MapStore<string, any>} [backingStore]
Copy link
Member Author

@erights erights Mar 7, 2024

Choose a reason for hiding this comment

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

This is a drive-by. The import from swingsets-liveslots is a layering violation that was not caught by our normal circularity checking, because the typing dependency apparently need not be represented by a package.json "dependency":. In any case, trying to import MapStore as import('@agoric/store').MapStore did not work, but the type MapStore seems to be in scope correctly anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Spoke too soon. Using a plain MapStore, yarn lint in @agoric/store works fine, but yarn lint in @agoric/vow` gives

../base-zone/src/make-once.js:11:12 - error TS2304: Cannot find name 'MapStore'.

11  * @param {MapStore<string, any>} [backingStore]

@michaelfig @turadg , help (not urgent). Is there a coherent explanation anywhere of these type scoping rules? My attempt to absorb them by example has only dug me into an ever deeper hole of confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Check your package's tsconfig.json and note under .compilerOptions.maxNodeModuleJsDepth there is an integer that says how deep Typescript should traverse into other modules for ambient types. If you increase that number, then it will work again.

That said, once we actually create the @endo versions of these packages, we should ensure they no longer rely on ambient types, and instead use explicit exports and imports. Turadg knows more about how this migration will happen, and it may be gated on (future) Typescript features to minimize the blast radius.

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'd love to do that. @turadg , is there an explanation of what the type scoping rules are once we've completely weaned ourselves of ambient types? Are we in fact waiting on future TS features before we make this conversion? How much of this would be done by your postponed Passable-typing endo PR? Does the new jsdoc support for better type import mean we will be able to stop repeating import('...'). at every use occurrence?

Copy link
Member

Choose a reason for hiding this comment

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

is there an explanation of what the type scoping rules are once we've completely weaned ourselves of ambient types?

Types will have to be explicitly imported into the module.

Are we in fact waiting on future TS features before we make this conversion?

There are no blockers. Mostly it hasn't been prioritized.

Does the new jsdoc support for better type import mean we will be able to stop repeating import('...'). at every use occurrence?

Yes, exactly. So waiting for TS 5.5 (https://github.com/microsoft/TypeScript/issues/57475) will reduce the downstream impact.

How much of this would be done by your postponed Passable-typing endo PR?

Totally orthogonal.

@dckc
Copy link
Member

dckc commented Mar 26, 2024

@erights want to try doing it in an example dapp first?

@erights
Copy link
Member Author

erights commented Mar 26, 2024

@erights want to try doing it in an example dapp first?

Yes, good idea.

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.

4 participants