reduce ambient imports and drop maxNodeModuleJsDepth #9373

turadg · 2024-05-15T22:28:17Z

refs: #6512

Description

further improves recent TypeScript best practices doc
turns the zoe/exported.js into a .d.ts so it can be used in triple-slash references
adopts that and stops the runtime imports of zoe/exported (progress on End runtime imports of types modules #6512)
remove the maxNodeModuleJsDepth now that the types resolve without
burns down imports so there are no inter-package runtime imports of typedefs. (leaving just a bit for End runtime imports of types modules #6512)

I put commit checkpoints of type coverage to guard against any regression, though I took them out again when merging

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages · 2024-05-15T22:32:24Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`8573fad`
Status:	✅ Deploy successful!
Preview URL:	https://6417c264.agoric-sdk.pages.dev
Branch Preview URL:	https://ta-no-ts-search.agoric-sdk.pages.dev

View logs

dckc

good stuff so far...

dckc · 2024-05-17T19:32:22Z

packages/agoric-cli/src/init.js

@@ -2,7 +2,6 @@ import chalk from 'chalk';
 import { makePspawn } from './helpers.js';

 /// <reference types="@endo/captp/src/types.js" />
-/// <reference types="@agoric/swingset-vat/exported.js" />


removing that file seems like a breaking change

for example, consider a dapp that imports it.

comment belongs on the deleted file, but I don't see how to do that.

true. in practice we know all the code that relies on it:
https://github.com/search?q=org%3AAgoric+swingset-vat%2Fexported&type=code

https://github.com/Agoric/dapp-pegasus/blob/f3b8f0400e9921a7497b08385d2b3aa609829182/api/deploy.js#L9

that looks ok to me to drop but if you want a BREAKING CHANGE commit I'm game. the package hasn't hit version 1 yet so it won't be a major version bump

In order to accommodate 3rd party developers, yes, I think a BREAKING CHANGE commit is worthwhile.

For example:

https://github.com/Kryha/KREAd/blob/c5c5d9e4b7cc370b0adfadc407ee7199c2064c98/agoric/contract/src/kreadKit.js#L43

None of the exported.js imported in that repo have been removed https://github.com/search?q=repo%3AKryha%2FKREAd%20exported&type=code

But a few have and I it'll be faster to do this than convince you otherwise ;-) I'll amend the commit message before I merge. I do wish we didn't have to modify commit history in order to note such things

oh. I misread this PR as deleting @agoric/zoe/exported.js.

dckc · 2024-05-17T19:34:04Z

packages/zoe/src/types-ambient.js

- * @property {Installation} installation
+ * @property {import('./zoeService/utils.js').Installation<any>} installation


I'd like to understand the motivation for this change better.

the Installation it was referencing before is available only ambiently. That one was just an alias for what's here

packages/zoe/src/types-ambient.js

dckc · 2024-05-17T19:37:17Z

packages/inter-protocol/src/psm/psm.js

-// XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/zoe/exported.js';
-import '@agoric/zoe/src/contracts/exported.js';
+/// <reference types="@agoric/zoe/exported" />


👏

I'm curious about the impact on bundle sizes for these contracts.

before

22135 bundle-econCommitteeCharter-js-meta.json 1166313 bundle-econCommitteeCharter.js 28423 bundle-fluxAggregatorKit-js-meta.json 1534301 bundle-fluxAggregatorKit.js 28121 bundle-psm-js-meta.json 1496517 bundle-psm.js

after

20614 bundle-econCommitteeCharter-js-meta.json 1105853 bundle-econCommitteeCharter.js 26868 bundle-fluxAggregatorKit-js-meta.json 1474173 bundle-fluxAggregatorKit.js 26566 bundle-psm-js-meta.json 1435797 bundle-psm.js

bundle abs drop rel drop

psm 60,720 4.1%

fluxAggregatorKit ~~98,504~~ 60,128 3.9%

econCommitteeCharter 60,460 5.2%

at 2844e7c

1104273 bundle-econCommitteeCharter.js 1472593 bundle-fluxAggregatorKit.js 1434213 bundle-psm.js

bundle abs drop rel drop

econCommitteeCharter 62,040 5.3%

fluxAggregatorKit 61,708 4.0%

psm 60,720 4.0%

dckc · 2024-05-17T19:47:53Z

packages/inter-protocol/src/vaultFactory/burn.js

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

 // XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/zoe/exported.js';


prune the XXX comment too?

I'll do that. Any other requested changes? I'm putting this in to R4R since all the tests are passing. (I don't know what's up with the Cloudflare build but it works for me locally)

mhofman

Looks like a nice cleanup, and I'm surprised we can simply use an exported.d.ts files that does imports of .js files which do the actual ambient side effects.

mhofman · 2024-05-18T04:55:51Z

packages/inter-protocol/src/vaultFactory/type-imports.js

-import '@agoric/zoe/exported.js';
-/// <reference path="./types.js" />
+/// <reference types="@agoric/zoe/exported" />
+/// <reference path="./types-ambient.js" />


I'm confused, I thought you couldn't reference .js files?

for reference types, but path works.

mhofman · 2024-05-18T04:56:40Z

packages/inter-protocol/src/vaultFactory/type-imports.js

How does this file work in the first place? I don't see it referenced anywhere

hm, yeah, some cruft. I'll remove

mhofman · 2024-05-18T05:01:30Z

packages/zoe/exported.js

-import '@agoric/ertp/exported.js';
-import '@agoric/store/exported.js';
-import '@agoric/swingset-vat/exported.js';
+// Dummy file for .d.ts twin to declare ambients


That seems like a cheap trick, what's the drawback of doing that everywhere?

Do you mean carrying out the best practice written here? https://github.com/Agoric/agoric-sdk/blob/master/docs/typescript.md#dts-modules

Or something more specific? If it's about exported.js, I'd like to move away from that pattern altogether. Zoe is just recalcitrant

dckc

ok... I think I get it now.

Though #6512 doesn't seem to be scheduled, I presume addressing it is worth the potential release branch complications. cc @gibson042 @mhofman

I'd be more comfortable if the schedule for releasing from master (#9252) were more clear. Again, I presume this progress is worth any downsides. cc @ivanlei

It didn't have any ambient exports. The named exports are in the package entrypoint.

turadg force-pushed the ta/use-.ts branch 2 times, most recently from a925bba to d371cb3 Compare May 16, 2024 16:31

Base automatically changed from ta/use-.ts to master May 16, 2024 18:18

turadg force-pushed the ta/no-ts-search branch from 4275436 to c1547b4 Compare May 17, 2024 19:19

turadg changed the title ~~drop maxNodeModuleJsDepth~~ reduce ambient imports and drop maxNodeModuleJsDepth May 17, 2024

turadg requested review from mhofman and dckc May 17, 2024 19:22

dckc reviewed May 17, 2024

View reviewed changes

turadg marked this pull request as ready for review May 17, 2024 20:44

turadg requested a review from dckc May 17, 2024 23:25

mhofman reviewed May 18, 2024

View reviewed changes

turadg requested a review from mhofman May 18, 2024 14:41

dckc approved these changes May 20, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label May 20, 2024

turadg added 2 commits May 20, 2024 19:54

docs: .d.ts benefits

0029140

chore(types): rm empty 'exported' module

2489715

turadg force-pushed the ta/no-ts-search branch from 2844e7c to c847ef0 Compare May 20, 2024 19:54

turadg added 12 commits May 20, 2024 12:59

chore: update type-coverage

a8f0167

refactor(types): .d.ts for exported

53f8f93

chore(types): reduce zoe runtime imports

c80fca2

chore(types): reduce zoe runtime imports

fc01e93

chore(types): end zoe/exported.js runtime import

0c30c37

lint(types): drop maxNodeModuleJsDepth to 1

1289650

lint(types): drop maxNodeModuleJsDepth (to zero)

d3273a5

chore(types): rm unused module

5bfe5cc

chore(types): no runtime imports from /store

f1112c5

chore(types): no runtime imports from /ertp

661042d

chore(types): no runtime imports from /notifier

1f02b5c

chore(types): no runtime imports of /zoe/contracts

a381b6a

turadg added 12 commits May 20, 2024 13:00

chore(types): no runtime imports of /network

a44411d

chore(types): no runtime imports of ses-ava

cb58a99

chore(types): no runtime imports of /vats

e47b0aa

chore(types): no runtime imports of /inter-protocol

4e65c28

chore(types): no runtime imports of /internal

543df85

refactor(types): ERTP exported.js only ambients

5bebd20

docs(typescript): where named and ambient exports go

4e207b9

chore(types): no import of local exported.js

089f06d

chore(types): drop noop network/exported.js

3367a51

It didn't have any ambient exports. The named exports are in the package entrypoint.

chore(types): rm unused import types-ambient

0930375

BREAKING CHANGE: drop exported.js

f825d5a

chore: update type-coverage

8573fad

turadg force-pushed the ta/no-ts-search branch from c847ef0 to 8573fad Compare May 20, 2024 20:08

mergify bot merged commit 980ff41 into master May 20, 2024
63 checks passed

mergify bot deleted the ta/no-ts-search branch May 20, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reduce ambient imports and drop maxNodeModuleJsDepth #9373

reduce ambient imports and drop maxNodeModuleJsDepth #9373

turadg commented May 15, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 15, 2024 •

edited

Loading

dckc left a comment

dckc May 17, 2024

turadg May 17, 2024

dckc May 20, 2024

turadg May 20, 2024 •

edited

Loading

dckc May 20, 2024

dckc May 17, 2024

turadg May 17, 2024

dckc May 17, 2024

turadg May 17, 2024 •

edited

Loading

turadg May 20, 2024

dckc May 17, 2024

turadg May 17, 2024

mhofman left a comment

mhofman May 18, 2024

turadg May 18, 2024

mhofman May 18, 2024

turadg May 18, 2024

mhofman May 18, 2024

turadg May 18, 2024

dckc left a comment

		* @property {Installation} installation
		* @property {import('./zoeService/utils.js').Installation<any>} installation

bundle	abs drop	rel drop
psm	60,720	4.1%
fluxAggregatorKit	~~98,504~~ 60,128	3.9%
econCommitteeCharter	60,460	5.2%

bundle	abs drop	rel drop
econCommitteeCharter	62,040	5.3%
fluxAggregatorKit	61,708	4.0%
psm	60,720	4.0%

reduce ambient imports and drop maxNodeModuleJsDepth #9373

reduce ambient imports and drop maxNodeModuleJsDepth #9373

Conversation

turadg commented May 15, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented May 15, 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

turadg May 20, 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

Choose a reason for hiding this comment

turadg May 17, 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

mhofman 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

dckc left a comment

Choose a reason for hiding this comment

turadg commented May 15, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 15, 2024 •

edited

Loading

turadg May 20, 2024 •

edited

Loading

turadg May 17, 2024 •

edited

Loading