group JSDoc imports #10665

turadg · 2024-12-10T17:31:47Z

part of #9005

Description

This adds a new rule @agoric/group-jsdoc-imports and enables it at warn level. That allows it to be autofixed, which this also does over the codebase. Then a few commits to clean things up.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Devs may see some warnings. They are pretty well explained

Testing Considerations

CI

Upgrade Considerations

n/a

cloudflare-workers-and-pages · 2024-12-10T17:31:49Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`9f7af83`
Status:	✅ Deploy successful!
Preview URL:	https://d3c8bc43.agoric-sdk.pages.dev
Branch Preview URL:	https://ta-eslint-plugin.agoric-sdk.pages.dev

View logs

@param

closes: #XXXX refs: #11173 #10665 ## Description We have two styles of type import in our js/jsdoc code. - at each point of use ```js @param {import('@agoric/base-zone').Zone} issuerZone ``` - once per module ```js @import {Zone} from '@agoric/base-zone'; ``` followed by using the simple name at each point of use ```js @import {Zone} from '@agoric/base-zone'; ``` We have not yet discussed this, so I am not yet proposing that we adopt one style over the other. However, for code that I'm editing, particularly when I need to think about these jsdoc-imported types, I find the "once per module style" easier to read and think about. It factors out into the header of the module all the inter-module type dependencies. Thus, for such modules, I "correct" these when I notice them. I noticed the ones in this PR while working on #11173. In addition, while I was at it, I also removed some unused type declarations and added in some missing ones. This should all be a pure refactoring. None should have observable runtime effect. - [x] file issues for actually resolving these as company-wide styles, and "enforce" them with additional lint rules. - At #11243 (comment) below, @turadg explains that he's already in progress on this at #10665 . With this, I do not expect any controversy on adopting this as a company-wide style rule, so am checking this box. Unfortunately, we need to go in the other direction, doing type imports at the point of use, in *.d.ts modules. See the comment at the top of packages/vats/src/core/types-ambient.d.ts ```js // Ambient type defs. Cannot use top-level import() because that would turn it into a module. ``` - [ ] Is there some way to factor out type imports in *.d.ts modules? ### Security Considerations Improvements in code understanding helps security. Especially about inter-package and inter-module dependencies. ### Scaling Considerations none ### Documentation Considerations - [ ] See what effects these changes have on the jsdoc-generated documentation ### Testing Considerations None ### Upgrade Considerations In addition to purely static type refactoring, this PR makes some runtime changes - It omits `makeMintAndIssuer` because this seems globally unused. - [x] We could instead deprecate it in this PR and only remove it later once we're really confident no one uses it. - We are proceeding with this deleted.

_unplanned_ ## Description #10665 has been around a while and I thought it would be good to land a custom eslint-plugin so we could add rules more easily. This implements a simple rule to fix type imports that are done using `@typedef` because those create new types, breaking the type nav and documentation available on hover. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations There's one reason to disable the rule, which is documented and demonstrated. ### Testing Considerations CI ### Upgrade Considerations n/a

Copilot

Pull Request Overview

This PR implements a new ESLint rule @agoric/group-jsdoc-imports that automatically moves inline JSDoc type imports (e.g., @type {import('@agoric/casting').FollowerOptions}) into a top-level JSDoc @import block, then references them by name (e.g., @type {FollowerOptions}). The rule is enabled at the warn level to allow autofixing, and the PR applies this autofix across the entire codebase.

Reviewed Changes

Copilot reviewed 300 out of 405 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/eslint-plugin/src/rules/group-jsdoc-imports.js	Implements the new ESLint rule for grouping JSDoc imports
packages/eslint-plugin/tests/rules/group-jsdoc-imports.test.js	Adds comprehensive tests for the new rule
packages/eslint-plugin/src/index.js	Registers the new rule and enables it in recommended config
packages/vats/src/core/demoIssuers.js	Applies the autofix to group JSDoc imports
packages/vat-data/src/exo-utils.js	Applies the autofix to group JSDoc imports
Multiple other files	Applies the autofix pattern across the codebase

mhofman

The change request is for the .ts files, which shouldn't get JSDoc comments. I suspect one case is a pre-existing incorrect @satisfies JSDoc, the other is for a @see which I'm not sure what the correct approach should be. In general, should this rule simply not apply to .ts files?

I am also concerned that using @import in files that declare ambient types will cause breakage, at least when .d.ts are generated and published.

Also, is there a rule to avoid duplicate @import comments with the same specifier? This seems to have created quite a few of those, and in general, I'd really like if we could lint to group multiple imports from the same file into a single directive. Not a blocker though.

packages/internal/src/types.ts

packages/portfolio-contract/test/supports.ts

packages/zoe/src/contracts/loan/types-ambient.js

packages/zoe/src/contracts/types-ambient.js

_hygiene_ ## Description This one got stale, - #11879 But we can at least cut out some tests and the obsolete CLI commands. That should help CI some and also reduce maintenance costs. E.g. for, - #10665 ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations CI ### Upgrade Considerations No on-chain changes. Tests are of chain code that has been taken out of service.

mhofman

This PR highlighted some preexisting typeof problems. I ended up opening #12202 based on this to address. Feel free to incorporate in this PR, or land it after,

is there a rule to avoid duplicate @import comments with the same specifier? This seems to have created quite a few of those, and in general, I'd really like if we could lint to group multiple imports from the same file into a single directive.

I created #12203 for a possible follow up. This should probably be a separate rule anyway.

mhofman · 2025-11-10T20:36:36Z

packages/inter-protocol/src/auction/auctionBook.js

 * @param {Baggage} baggage
 * @param {ZCF} zcf
- * @param {MakeRecorderKit} makeRecorderKit
+ * @param {import('@agoric/zoe/src/contractSupport/recorder.js').MakeRecorderKit} makeRecorderKit


Why keep/revert to inline import() here?

packages/inter-protocol/test/auction/tools.js

mhofman · 2025-11-11T00:06:07Z

packages/eslint-plugin/tests/rules/group-jsdoc-imports.test.js

+ */
+
+/**
+ * @import {SnapStoreInternal} from './snapStore.js';


Out of curiosity, why place the @import in this block and not the other block that already contains an @import ?

Just expedient

mhofman · 2025-11-11T00:07:24Z

packages/swing-store/src/assertComplete.js

+/**
+ * @import {SwingStoreInternal} from './internal.js';
+ * @import {ArtifactMode} from './internal.js';
+ */


A little strange to see the @import after the usage. I suspect because there is no preamble in this file?

packages/SwingSet/src/controller/controller.js

mhofman · 2025-11-11T00:15:17Z

packages/SwingSet/src/controller/controller.js

- * @import {SlogSender} from '@agoric/telemetry';
- * @import {VatWarehousePolicy} from '../types-external.js';
- * @import {spawn} from 'child_process';
- * @import {makeXsnapBundleData} from './bundle-handler.js';
- * @import {BundleHandler} from './bundle-handler.js';
- * @import {default} from '../kernel/kernel.js';


Out of curiosity, why revert these and go back to inline import?

the default doesn't work

Interesting that default is a problem with @import.

Why revert the other imports?

to save time making line edits vs reverting on the CLI. this is a super lower priority project.

packages/inter-protocol/test/smartWallet/oracle-integration.test.js

packages/pola-io/src/ambient/cmd.js

refs: #10665 ## Description Reviewing #10665 made me realize we have a lot of broken type declarations using runtime values without `typeof`. These technically resolve to `any`, but sometimes tsc is smart enough to make the `typeof` implicit. It's hard to tell since we're not running tsc in strict mode (aka with implicit any allowed). This is mostly the result of a quick grep, and manual review, so I expect I missed some cases. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations I expect existing tests to keep passing. I haven't checked coverage before/after. ### Upgrade Considerations None

refs: #10665 Reviewing #10665 made me realize we have a lot of broken type declarations using runtime values without `typeof`. These technically resolve to `any`, but sometimes tsc is smart enough to make the `typeof` implicit. It's hard to tell since we're not running tsc in strict mode (aka with implicit any allowed). This is mostly the result of a quick grep, and manual review, so I expect I missed some cases. None None None I expect existing tests to keep passing. I haven't checked coverage before/after. None

mhofman · 2025-11-11T23:36:04Z

It seems the latest changes re-introduced @import in packages/inter-protocol/src/vaultFactory/types-ambient.js and reverted typeof changes in packages/inter-protocol/src/proposals/econ-behaviors.js. Is that expected?

either ambients or the autofix doesn't work well

refs: #10665 Reviewing #10665 made me realize we have a lot of broken type declarations using runtime values without `typeof`. These technically resolve to `any`, but sometimes tsc is smart enough to make the `typeof` implicit. It's hard to tell since we're not running tsc in strict mode (aka with implicit any allowed). This is mostly the result of a quick grep, and manual review, so I expect I missed some cases. None None None I expect existing tests to keep passing. I haven't checked coverage before/after. None

turadg force-pushed the ta/eslint-plugin branch from b4a8a4f to f041d17 Compare December 29, 2024 05:12

turadg mentioned this pull request Jan 7, 2025

require that promise-returning functions that could throw are async #10809

Open

turadg mentioned this pull request Apr 23, 2025

refactor: move some type imports to @import #11243

Merged

4 tasks

turadg force-pushed the ta/eslint-plugin branch 2 times, most recently from 9f7af83 to 9305ae1 Compare May 14, 2025 16:23

turadg added the bypass:integration Prevent integration tests from running on PR label May 14, 2025

turadg mentioned this pull request Sep 17, 2025

custom eslint plugin that fixes typedef imports #11949

Merged

turadg force-pushed the ta/eslint-plugin branch 2 times, most recently from 404be7c to 9ab5015 Compare November 7, 2025 22:20

turadg changed the title ~~eslint-plugin for Agoric SDK~~ group JSDoc imports Nov 7, 2025

turadg requested review from Copilot, kriskowal and mhofman November 7, 2025 22:25

Copilot AI reviewed Nov 7, 2025

View reviewed changes

turadg marked this pull request as ready for review November 7, 2025 22:27

turadg added the automerge:rebase Automatically rebase updates, then merge label Nov 7, 2025

mhofman requested changes Nov 7, 2025

View reviewed changes

mhofman removed the bypass:integration Prevent integration tests from running on PR label Nov 7, 2025

turadg marked this pull request as draft November 7, 2025 23:11

turadg requested a review from mhofman November 10, 2025 20:27

mhofman added the force:integration Force integration tests to run on PR label Nov 10, 2025

turadg removed the automerge:rebase Automatically rebase updates, then merge label Nov 10, 2025

turadg marked this pull request as ready for review November 10, 2025 22:09

turadg mentioned this pull request Nov 10, 2025

remove tests and ops for auctions #12200

Merged

mhofman mentioned this pull request Nov 11, 2025

chore(types): typeof for runtime types #12202

Merged

mhofman approved these changes Nov 11, 2025

View reviewed changes

feat: rule to group JSDoc imports

ac7746f

turadg force-pushed the ta/eslint-plugin branch from 9147722 to 027a93f Compare November 11, 2025 22:44

turadg force-pushed the ta/eslint-plugin branch from 027a93f to d25bc69 Compare November 11, 2025 23:17

turadg added the automerge:rebase Automatically rebase updates, then merge label Nov 11, 2025

turadg added 13 commits November 11, 2025 16:02

lint: disable group-jsdoc-imports for some files

dc0b512

either ambients or the autofix doesn't work well

lint: fix group-jsdoc-imports

96ce636

lint: manual fixups

75bde97

chore(types): remove conflicting ambients

906926d

chore(types): fix jsdoc types in .ts

1874707

lint: fix group-jsdoc-imports

eb504ed

lint: manual fixups

5ef2f20

lint: fix group-jsdoc-imports

978cbf4

lint: fix group-jsdoc-imports

97dc6ce

lint: fix group-jsdoc-imports

19de718

lint: fix group-jsdoc-imports

811522f

lint: manual fixups

986e318

lint: fixup start fns

290cd68

turadg force-pushed the ta/eslint-plugin branch from d25bc69 to fefed2c Compare November 12, 2025 00:12

mhofman force-pushed the ta/eslint-plugin branch from fefed2c to 24c2b95 Compare November 12, 2025 00:25

lint: manual fixups

f675cdc

mergify bot merged commit 64cee5a into master Nov 12, 2025
94 checks passed

mergify bot deleted the ta/eslint-plugin branch November 12, 2025 01:26

group JSDoc imports #10665

group JSDoc imports #10665

Uh oh!

Conversation

turadg commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Uh oh!

cloudflare-workers-and-pages bot commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying agoric-sdk with Cloudflare Pages

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

mhofman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhofman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhofman commented Nov 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

turadg commented Dec 10, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 10, 2024 •

edited

Loading

mhofman left a comment •

edited

Loading