explicit type exports for agoric/notifier #9305

turadg · 2024-04-29T16:53:55Z

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

Scaling Considerations

none, types

Documentation Considerations

might improve API docs of Notifier package

Testing Considerations

CI

Upgrade Considerations

none, types

cloudflare-workers-and-pages · 2024-04-29T17:03:18Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`001d8e4`
Status:	✅ Deploy successful!
Preview URL:	https://d16dd55e.agoric-sdk.pages.dev
Branch Preview URL:	https://ta-notifier-types.agoric-sdk.pages.dev

View logs

dckc

I don't see any critical problems, but I'm not confident about approving yet; I'd like to hear more about these comments...

packages/cache/src/main.js

dckc · 2024-04-29T17:01:03Z

packages/notifier/exported.d.ts

+import {
+  EachTopic as _EachTopic,


somewhere I saw a good explanation of why these gymnastics are used, including a stackoverflow reference; is it convenient to add a pointer from here?

I resisted coping it an Nth time. I'll write up some docs in Markdown and have the modules reference it

dckc · 2024-04-29T17:04:12Z

packages/notifier/package.json

-    "prepack": "echo \"export {}; \" | cat - src/types-ambient.js > src/types.js && tsc --build tsconfig.build.json",
+    "prepack": "tsc --build tsconfig.build.json",


👏 good riddance

this is the PR's raison d'être

dckc · 2024-04-29T17:07:09Z

packages/notifier/src/index.js

@@ -1,5 +1,7 @@
 // @jessie-check

+import '@agoric/internal/exported.js';


is it worth an XXX or something on these? or UNTIL #6512 ?

there are tons of these but I tried to comment a bunch of them

dckc · 2024-04-29T17:07:39Z

packages/notifier/src/index.js

@@ -26,3 +28,6 @@ export {
 } from './asyncIterableAdaptor.js';
 export * from './storesub.js';
 export * from './stored-notifier.js';
+
+// eslint-disable-next-line import/export -- doesn't know types
+export * from './types.js';


so this is a solution to the question of re-exporting?

this is the right way to export at the top level. unfortunately it doesn't work for @agoric/ertp because it has AssetKind as both a type and kind

interesting.

considers an issue to change the name of the type to AssetKindT

dckc · 2024-04-29T17:09:08Z

packages/notifier/src/stored-notifier.js

+/**
+ * @import {ERef} from '@endo/far';
+ * @import {BaseNotifier, Notifier, StoredFacet} from './types.js';
+ * @import {Marshaller, StorageNode, Unserializer} from '@agoric/internal/src/lib-chainStorage.js';


having StorageNode in @agoric/internal looks more and more awkward.

yeah. and @agoric/notifier depending on ChainStorage.

"description": "Notifier allows services to update clients about state changes using a stream of promises",

dckc · 2024-04-29T17:10:29Z

packages/notifier/src/storesub.js

@@ -97,7 +103,7 @@ export const makeStoredSubscription = (
    serializeBodyFormat: 'smallcaps',
  }),
 ) => {
-  /** @type {Unserializer} */
+  /** @type {import('@agoric/internal/src/lib-chainStorage.js').Unserializer} */


The @import on line 13 didn't work?

just extra chars. fixed

dckc · 2024-04-29T17:14:04Z

packages/notifier/test/test-notifier.js

+
+/**
+ * @import {ERef} from '@endo/far';
+ * @import {IterationObserver, LatestTopic, Notifier, NotifierRecord, PublicationRecord, Publisher, PublishKit, StoredPublishKit, StoredSubscription, StoredSubscriber, Subscriber, Subscription, UpdateRecord} from '../src/types.js';


These long lists make me wonder about qualified imports.

yeah. .ts and its tooling makes the long list easy to work with but for JSDoc we might be better off with a name per module

dckc · 2024-04-29T17:15:46Z

packages/notifier/test/test-stored-subscription.js

-import '../src/types-ambient.js';
+import '../src/types.js';


I'm surprised that this works. types.js is now a module, so types defined there are not ambient, right?

it's a noop but I'll remove it

dckc

ok, none of my questions turned up answers that I'm not comfortable with.

dckc

looks good.

Review by commit gives me reasonable confidence that conflicts with release branches should be manageable.

dckc · 2024-04-30T19:02:24Z

packages/SwingSet/src/types-ambient.js

- * WaitUntilQuiescent,
- * XSnap,
- * } from './types-external.js')
+ * @import { Bundle, BundleCap, BundleFormat, BundleID, BundleRef, BundleSpec, DeviceInvocation, DeviceInvocationResult, DeviceInvocationResultError, DeviceInvocationResultOk, EndoZipBase64Bundle, GetExportBundle, KernelDeliveryBringOutYourDead, KernelDeliveryChangeVatOptions, KernelDeliveryDropExports, KernelDeliveryMessage, KernelDeliveryNotify, KernelDeliveryObject, KernelDeliveryOneNotify, KernelDeliveryRetireExports, KernelDeliveryRetireImports, KernelDeliveryStartVat, KernelKeeper, KernelOneResolution, KernelOptions, KernelSlog, KernelSyscallDropImports, KernelSyscallExit, KernelSyscallInvoke, KernelSyscallObject, KernelSyscallResolve, KernelSyscallResult, KernelSyscallResultError, KernelSyscallResultOk, KernelSyscallRetireExports, KernelSyscallRetireImports, KernelSyscallSend, KernelSyscallSubscribe, KernelSyscallVatstoreDelete, KernelSyscallVatstoreGet, KernelSyscallVatstoreGetNextKey, KernelSyscallVatstoreSet, ManagerType, Message, MeteringVatPowers, NestedEvaluateBundle, PolicyInput, PolicyInputCrankComplete, PolicyInputCrankFailed, PolicyInputCreateVat, PolicyInputNone, PolicyOutput, ResolutionPolicy, RunPolicy, SlogFinishDelivery, SlogFinishSyscall, SnapshotResult, SnapStore, SourceOfBundle, SourceSpec, StaticVatPowers, SwingSetCapData, SwingSetConfig, SwingSetConfigDescriptor, SwingSetConfigProperties, SwingSetKernelConfig, TerminationVatPowers, VatAdminSvc, VatKeeper, VatPowers, VatSlog, VatStats, WaitUntilQuiescent, XSnap, } from './types-external.js';


does this work?
IME, @import makes this act like a module and hence not produce ambient types.
But I guess it was that way before this PR.

I don't know all the cases yet but in this one the ambient work, maybe because swingset-vats never builds .d.ts.

When I get it to build types-ambient.d.ts it's empty:

//# sourceMappingURL=types-ambient.d.ts.map

dckc · 2024-04-30T19:17:41Z

packages/network/src/network.js

-   * @param {Remote<ProtocolImpl>} opts.protocolImpl
+   * @param {import('@agoric/vow').Remote<ProtocolImpl>} opts.protocolImpl


importing Remote once didn't work?

refs: #9305 ## Description While working on #9305 I was slowed down by the `types.js` files that were really ambients. This makes all `types.js` that aren't modules into `types-ambient.js`. The ambients for `zoe/tools` I ditched by making them explicit exports, to continue supporting that part of the package API that the `prepack` rename was doing. ### Security Considerations  ### Scaling Considerations  ### Documentation Considerations  ### Testing Considerations  ### Upgrade Considerations

turadg requested review from dckc and samsiegart April 29, 2024 16:53

dckc reviewed Apr 29, 2024

View reviewed changes

turadg requested a review from dckc April 29, 2024 18:25

dckc approved these changes Apr 29, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label Apr 29, 2024

turadg force-pushed the ta/notifier-types branch 2 times, most recently from 53f5b00 to 20b9c5e Compare April 30, 2024 00:22

turadg added force:integration Force integration tests to run on PR and removed automerge:rebase Automatically rebase updates, then merge labels Apr 30, 2024

turadg requested a review from dckc April 30, 2024 18:57

dckc approved these changes Apr 30, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label Apr 30, 2024

turadg force-pushed the ta/notifier-types branch 3 times, most recently from cac8216 to ff95b42 Compare April 30, 2024 23:10

turadg added 13 commits April 30, 2024 16:41

feat(types): ambient exports from agoric/internal

71d18c4

feat(types): explicit exports from notifier

0bc72a8

chore(types): dedupe

2797d60

chore(types): normalize exported.d.ts

514c7fa

docs: XXX for 6512

12490a5

test: fix arity

5ed655d

refactor(types): inline

f41ff2a

chore(type): work around '* Bundle' type bug

7d634c5

refactor: consistent types-ambient

067a206

test: accomodate types

0b04527

fix(types): netstring decode

647afb6

chore(types): ambient VatAdminScv

f70d226

feat(types): explicit exports from network

65c2075

chore(types): accomodate export changes

001d8e4

turadg force-pushed the ta/notifier-types branch from ff95b42 to 001d8e4 Compare April 30, 2024 23:42

mergify bot merged commit e456308 into master May 1, 2024
67 checks passed

mergify bot deleted the ta/notifier-types branch May 1, 2024 00:13

turadg mentioned this pull request May 1, 2024

normalize types-ambient #9309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

explicit type exports for agoric/notifier #9305

explicit type exports for agoric/notifier #9305

turadg commented Apr 29, 2024

cloudflare-workers-and-pages bot commented Apr 29, 2024 •

edited

Loading

dckc left a comment

dckc Apr 29, 2024

turadg Apr 29, 2024

dckc Apr 29, 2024

turadg Apr 29, 2024

dckc Apr 29, 2024

turadg Apr 29, 2024

dckc Apr 29, 2024

turadg Apr 29, 2024

dckc Apr 29, 2024

dckc Apr 29, 2024

turadg Apr 29, 2024

dckc Apr 29, 2024

turadg Apr 29, 2024 •

edited

Loading

dckc Apr 29, 2024

turadg Apr 29, 2024

dckc Apr 29, 2024

turadg Apr 29, 2024 •

edited

Loading

dckc left a comment

dckc left a comment

dckc Apr 30, 2024

turadg Apr 30, 2024 •

edited

Loading

dckc Apr 30, 2024

		"prepack": "echo \"export {}; \" \| cat - src/types-ambient.js > src/types.js && tsc --build tsconfig.build.json",
		"prepack": "tsc --build tsconfig.build.json",

		@@ -1,5 +1,7 @@
		// @jessie-check

		import '@agoric/internal/exported.js';

		* @param {Remote<ProtocolImpl>} opts.protocolImpl
		* @param {import('@agoric/vow').Remote<ProtocolImpl>} opts.protocolImpl

explicit type exports for agoric/notifier #9305

explicit type exports for agoric/notifier #9305

Conversation

turadg commented Apr 29, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Apr 29, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Apr 29, 2024 •

edited

Loading

turadg Apr 29, 2024 •

edited

Loading

turadg Apr 29, 2024 •

edited

Loading

turadg Apr 30, 2024 •

edited

Loading