fix(marshal): remove ambient types, generate declarations from JSDoc #1025

michaelfig · 2022-01-28T04:34:02Z

Experiment to remove dependency on ambient types. The process:

Put export {}; near the top of every JSDoc types.js file. This switches it from an ambient file to a module with exported types.
Remove exported.js so that ambient type users break hard.
Add export * from './src/types.js'; to index.js so that upstream consumers can import types from the package identifier. Season with eslint-ignores to taste.
Remove any import './types.js';, import './internal-types.js'; etc.
Add /** @typedef {import('./types.js').MemberName} MemberName */ or for template types /** @template T @typedef {import('./types.js').MemberName2<T>} MemberName2 */. as needed to the top of .js sources.
Chase any other Typescript errors.
Add build step to generate declarations, modify package.json to include generated files.
In consumers of the package, add JSDoc typedef imports like /** @typedef {import('@agoric/marshal').MyType} MyType */, and remove any import '@agoric/marshal/exported';.
When all cross project dependencies have transitioned to explicitly exported type declarations, remove any --maxNodeModulesJsDepth hack that used to be needed with JSDoc ambient type in node_modules.

mhofman

Cool stuff! I'd be so much more confident in this kind of changes if we had noImplicitAny.

Requesting change as I believe the default convert slot got flipped.

packages/marshal/src/deeplyFulfilled.js

mhofman · 2022-01-28T04:46:56Z

packages/marshal/src/deeplyFulfilled.js

 import { isPromise } from '@endo/promise-kit';
 import { getTag, isObject } from './helpers/passStyle-helpers.js';
 import { makeTagged } from './makeTagged.js';
 import { passStyleOf } from './passStyleOf.js';

+/** @typedef {import('./types').Passable} Passable */


Nit: while tsc doesn't care as much, I think we should keep the habit of using fully qualified names, aka with extensions, for imports

Reasons to leave it this way:

There should never be any ambiguity what file it refers to. (There must not be a types.ts next to a types.js.)

We can change the syntax of the imported file between .ts and .js without touching everywhere it's imported.

Even if there is a .d.ts or .ts file, the import should always be of .js file per TypeScript conventions. TypeScript will internally use the .d.ts file if it exists. We use explicit .js extensions for our imports as automatic extension resolution is something we left behind when switching to native ESM.

Thanks for the correction on the second bullet about changing extensions. I didn't know that TypeScript conventions are that if the file is foo.ts you should still import it as foo.js.

I neglected to include what I think is the most compelling reason to leave it this way: less to type and read. I think simplicity wins unless there is a compelling reason to add the extension. Is it to be consistent with the JSM module imports? I think consistency is important when it reduces confusion or misunderstanding, but I have a hard time imagining someone being confused by the absence of .js here in the JSDoc.

Right I was looking for consistency. I've never found terseness to be very compelling. We require extensions for ESM imports (static, and in the future dynamic), and since TS style jsdoc type imports use a dynamic-like ESM notion, I figure we should stay consistent.

packages/marshal/src/helpers/internal-types.js

packages/marshal/src/helpers/passStyle-helpers.js

packages/marshal/src/marshal.js

erights

A worthy experiment. It is much less painful than I expected. Enough so that, assuming this is representative, I'm ok moving in this direction in general.

vscode has a very nice feature that when I use a name I have not yet imported but should, it often (not always) prompts me with the correct choice and writes the import itself. If it did so for this /** @typedef {import('../types').Foo} Foo */ pattern as well, then I wouldn't even hesitate. Maybe eventually we'll have such tooling.

Thanks for pushing ahead with this. A very pleasant surprise. LGTM!!!

packages/marshal/src/helpers/passStyle-helpers.js

packages/marshal/src/marshal-justin.js

erights · 2022-01-28T04:46:39Z

packages/marshal/src/marshal.js

-export function makeMarshal(
-  convertValToSlot = defaultValToSlotFn,
-  convertSlotToVal = defaultSlotToValFn,
+export const makeMarshal = (


I missed that one. Thanks!

Have we considered a global code mod to convert functions? https://github.com/facebook/jscodeshift works great

Not yet, but maybe sometime.

I would love to see this done automatically!

an attempt: Agoric/agoric-sdk#4608

packages/marshal/src/marshal.js

packages/marshal/src/types.js

packages/marshal/test/test-marshal-far-obj.js

mhofman · 2022-01-28T05:02:16Z

vscode has a very nice feature that when I use a name I have not yet imported but should, it often (not always) prompts me with the correct choice and writes the import itself.

It does for import type in .ts files, but unfortunately I'm not aware of a similar jsdoc style.

One drawback I believe is that going to the type definition would take 2 clicks, first one going to the typedef alias.

turadg · 2022-01-28T19:42:18Z

packages/marshal/exported.js

+// eslint-disable-next-line import/export
+export * from './src/types.js';


+1 to this pattern of explicit type exports over implicit.

We shouldn't have to disable the linter to do the right thing. https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/export.md says it prevents "funny business"; doesn't TypeScript cover that? I propose we consider disabling this rule in our eslint config.

@michaelfig, could we not include this export * from './src/types.js'; in the index.js, so that we can get rid of the exported.js? I believe the only purpose it served was for making sure marshal ambient types were loaded, which we want to get rid of, right?

I have gone ahead and done that. It reads much better.

turadg · 2022-01-28T19:48:09Z

packages/marshal/src/helpers/passStyle-helpers.js

+/** @typedef {import('../types').Checker} Checker */
+/** @typedef {import('../types').PassStyle} PassStyle */


If we're sticking with JSDoc, maybe we can submit a PR for TypeScript to have a nicer syntax for this.

In regular TS this is clean:

import type {Checker, PassStyle} from '../types`

In JSDoc TS maybe,

/** * @typeimport '../types' {Checker, PassStyle} */

Not the first to want this :) microsoft/TypeScript#22160

turadg

LGTM. I left some non-blocking comments.

Glad to see support for type imports, which came up in Agoric/agoric-sdk#4410 (comment)

packages/marshal/src/marshal-justin.js

turadg · 2022-01-28T19:52:48Z

packages/marshal/src/marshal.js

-export function makeMarshal(
-  convertValToSlot = defaultValToSlotFn,
-  convertSlotToVal = defaultSlotToValFn,
+export const makeMarshal = (


Have we considered a global code mod to convert functions? https://github.com/facebook/jscodeshift works great

packages/marshal/src/marshal.js

packages/marshal/src/types.js

mhofman · 2022-02-02T05:19:15Z

I took over the PR and extended it with generation of declaration files for marshal, and rebased on latest master + preliminary TS 4.5 compat PR I opened in #1043. I've checked that with the appropriate updates to Agoric/agoric-sdk#4413, all seem to be working well. This shows how we can move away from ambient types, and stop requiring TS from trying to parse the JS of node_modules to figure out typing. With this we should be able to publish packages that are more compatible with the ecosystem.

packages/marshal/src/make-far.js

turadg

Left some non-blocking suggestions and questions.

packages/marshal/src/helpers/passStyle-helpers.js

packages/marshal/src/make-far.js

packages/marshal/src/marshal.js

packages/marshal/src/types.js

packages/marshal/tsconfig.json

mhofman · 2022-02-02T22:47:15Z

Reverted some of the changes so this PR is now purely type changes (and a couple function foo() {} -> const foo = () => {})

kriskowal

Moving away from ambient types is a good but breaking change. This will require some ceremony.

In each affected NEWS.md, we need a note for # Next release of what end-users will need to do when they upgrade.

At least one commit must have ! in the conventional commit and a *BREAKING CHANGE*: ... paragraph that will get rolled into the changelog and bump the version appropriately.

When integrating into Agoric SDK, I use scripts/sync-versions.sh ../endo and that rolls over all the semver bumps, so all of the alignment will have to occur in the Sync Endo PR.

kriskowal · 2022-02-03T22:07:35Z

In the commit message, BREAKING CHANGE: must be exactly *BREAKING CHANGE*:for Lerna’s conventional commit changelog thing to pick it up.

kriskowal

Please address my comment about the commit message having *BREAKING CHANGE:* verbatim. Otherwise, this is great progress.

mhofman · 2022-02-03T22:37:53Z

Please address my comment about the commit message having *BREAKING CHANGE:* verbatim. Otherwise, this is great progress.

Done.

@michaelfig could you re-review as a comment? I'll carry your review as mine (since you are the original author...)

michaelfig

Looking good! Just a few minor cleanups I'd recommend, but not blocking.

packages/marshal/package.json

packages/marshal/src/types.js

Keep using JSdoc, remove exported.js *BREAKING CHANGE*: This change switches to exported types and removes the `exported.js` file which provided ambient types.

Also add NEWS.md in artifact

mhofman

Carrying @michaelfig's approval

turadg · 2022-02-19T17:10:29Z

I just noticed that building the d.ts files (e.g. by running prepack) spits them out adjacent to the .js files, which aren't gitignored and could end up being checked in.

Any reasons not to output the build into a dist directory that is ignored?

mhofman · 2022-02-19T17:14:08Z

Because we'd need to also copy the .js source, which would be incompatible with non-publish same repo usage

turadg · 2022-02-19T17:22:45Z

Because we'd need to also copy the .js source, which would be incompatible with non-publish same repo usage

We'd only need to copy the .js source if we pointed "main": at dist but that can be left alone by adding "types": "dist/…".

A problem with how it is now (besides the gitignore) is that src/foo.js and src/foo.ts can be out of sync. If build output went to a dist/ then you'd have a guarantee that .ts adjacent to its source .js matched.

mhofman · 2022-02-19T17:33:04Z

I am not ok with anything that requires same repo build step to have anything work. It's a foot gun.

How can they be out of sync? They're only generated when publishing a package, and not needed same repo as tsc will happily parse the JS to get type definitions when the dependency is not in node_modules

turadg · 2022-02-19T17:45:11Z

I am not ok with anything that requires same repo build step to have anything work. It's a foot gun.

I'm not familiar with the foot injuries you have in mind. In my experience watch mode suffices to keep things up to date.

How can they be out of sync? They're only generated when publishing a package

They're generated when publishing a package, but don't go away on their own. Then the source .js can be edited and is out of sync. Not "expected", but perhaps a "foot gun".

mhofman · 2022-02-19T17:51:39Z

They're generated when publishing a package, but don't go away on their own

True, we need a clean step after publish, and / or move publishing to CI.

Watch step requires developer environment setup. I think the fact these build artifacts are not ignored is probably a good thing as it'll remind a clean is needed.

kriskowal · 2022-02-19T18:17:13Z

I would very much prefer not to need a watch mode.

michaelfig · 2022-02-19T19:51:31Z

They're generated when publishing a package, but don't go away on their own

@turadg, there's a postpack that does a clean. Are you saying that doesn't work for you?

kriskowal · 2022-02-19T23:29:24Z

They're generated when publishing a package, but don't go away on their own

@turadg, there's a postpack that does a clean. Are you saying that doesn't work for you?

After my last publish, the .d.ts and .map files were left behind.

michaelfig · 2022-02-20T00:33:57Z

I find with the following, things work as they should (at least wrt. clean). Explain again why we're including *.ts in our jsconfig.jsons?

diff --git a/packages/marshal/jsconfig.json b/packages/marshal/jsconfig.json
index b455fcf0d..0611838b4 100644
--- a/packages/marshal/jsconfig.json
+++ b/packages/marshal/jsconfig.json
@@ -15,5 +15,5 @@
     "strictNullChecks": true,
     "moduleResolution": "node",
   },
-  "include": ["*.js", "*.ts", "src/**/*.js", "src/**/*.ts", "test/**/*.js"]
+  "include": ["*.js", "src/**/*.js", "test/**/*.js"]
 }

kriskowal · 2022-02-20T00:40:10Z

That’s good to know. That patch is acceptable. I made the change to cover .ts files in general across the repository, since I found that hand-written definitions were not covered by tests, which caused failures to only be observable when integrated in a dependee.

michaelfig · 2022-02-20T00:44:06Z

I made the change to cover .ts files in general across the repository, since I found that hand-written definitions were not covered by tests, which caused failures to only be observable when integrated in a depended.

Maybe the correct solution then is to do:

diff --git a/packages/marshal/jsconfig.build.json b/packages/marshal/jsconfig.build.json
index 13018509f..ea9465e90 100644
--- a/packages/marshal/jsconfig.build.json
+++ b/packages/marshal/jsconfig.build.json
@@ -6,5 +6,5 @@
     "emitDeclarationOnly": true,
     "declarationMap": true
   },
-  "exclude": ["test/"]
+  "exclude": ["test/", "**/*.ts"]
 }

michaelfig requested a review from kriskowal January 28, 2022 04:34

michaelfig mentioned this pull request Jan 28, 2022

Import explicit types from @endo/marshal Agoric/agoric-sdk#4413

Closed

mhofman requested changes Jan 28, 2022

View reviewed changes

erights approved these changes Jan 28, 2022

View reviewed changes

turadg reviewed Jan 28, 2022

View reviewed changes

turadg approved these changes Jan 28, 2022

View reviewed changes

mhofman changed the base branch from master to mhofman/ts-45-compatibility February 2, 2022 05:06

mhofman force-pushed the mfig-marshal-explicit-jsdoc branch from f7345c8 to b55a2be Compare February 2, 2022 05:12

mhofman self-requested a review February 2, 2022 05:35

mhofman reviewed Feb 2, 2022

View reviewed changes

packages/marshal/src/make-far.js Show resolved Hide resolved

mhofman changed the title ~~fix(marshal): remove ambient types, but still use JSDoc~~ fix(marshal): remove ambient types, generate declarations from JSDoc Feb 2, 2022

mhofman self-assigned this Feb 2, 2022

mhofman requested a review from turadg February 2, 2022 15:51

turadg approved these changes Feb 2, 2022

View reviewed changes

mhofman self-requested a review February 2, 2022 17:21

mhofman force-pushed the mhofman/ts-45-compatibility branch 4 times, most recently from 4ecf8ab to 2c0bff0 Compare February 2, 2022 21:56

mhofman force-pushed the mfig-marshal-explicit-jsdoc branch 2 times, most recently from c5f2ef7 to b0d11fa Compare February 2, 2022 22:43

kriskowal approved these changes Feb 2, 2022

View reviewed changes

kriskowal requested changes Feb 2, 2022

View reviewed changes

mhofman force-pushed the mhofman/ts-45-compatibility branch 3 times, most recently from 2bbf300 to 8513cfb Compare February 3, 2022 16:33

mhofman force-pushed the mfig-marshal-explicit-jsdoc branch 2 times, most recently from 4a62453 to be9d5ac Compare February 3, 2022 21:38

mhofman mentioned this pull request Feb 3, 2022

fix: quick "fix" of a red squiggle problem Agoric/agoric-sdk#4447

Merged

kriskowal approved these changes Feb 3, 2022

View reviewed changes

mhofman force-pushed the mfig-marshal-explicit-jsdoc branch from be9d5ac to ac69e63 Compare February 3, 2022 22:36

michaelfig commented Feb 3, 2022

View reviewed changes

packages/marshal/package.json Outdated Show resolved Hide resolved

packages/marshal/src/types.js Outdated Show resolved Hide resolved

mhofman added 2 commits February 4, 2022 00:04

fix(marshal)!: Remove ambient types

2a9cf63

Keep using JSdoc, remove exported.js *BREAKING CHANGE*: This change switches to exported types and removes the `exported.js` file which provided ambient types.

chore(marshal): switch to exported declarations

5235b7f

Also add NEWS.md in artifact

mhofman force-pushed the mfig-marshal-explicit-jsdoc branch from 1e0194b to 5235b7f Compare February 4, 2022 00:05

mhofman approved these changes Feb 4, 2022

View reviewed changes

mhofman enabled auto-merge February 4, 2022 00:08

mhofman merged commit 47e3daa into master Feb 4, 2022

mhofman deleted the mfig-marshal-explicit-jsdoc branch February 4, 2022 00:12

mhofman mentioned this pull request Feb 16, 2022

export external types as modules Agoric/agoric-sdk#4560

Open

mhofman mentioned this pull request Apr 15, 2022

Investigate removal of generated .d.ts.map files #1162

Closed

turadg mentioned this pull request Aug 16, 2022

export explicit types #1254

Open

38 tasks

		// eslint-disable-next-line import/export
		export * from './src/types.js';

		/** @typedef {import('../types').Checker} Checker */
		/** @typedef {import('../types').PassStyle} PassStyle */

fix(marshal): remove ambient types, generate declarations from JSDoc #1025

fix(marshal): remove ambient types, generate declarations from JSDoc #1025

Conversation

michaelfig commented Jan 28, 2022 • edited by mhofman Loading

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

erights 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

mhofman commented Jan 28, 2022

turadg Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

mhofman Feb 3, 2022 • 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Feb 2, 2022

turadg left a comment

Choose a reason for hiding this comment

mhofman commented Feb 2, 2022

kriskowal left a comment

Choose a reason for hiding this comment

kriskowal commented Feb 3, 2022

kriskowal left a comment

Choose a reason for hiding this comment

mhofman commented Feb 3, 2022

michaelfig left a comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

turadg commented Feb 19, 2022

mhofman commented Feb 19, 2022

turadg commented Feb 19, 2022 • edited Loading

mhofman commented Feb 19, 2022

turadg commented Feb 19, 2022

mhofman commented Feb 19, 2022

kriskowal commented Feb 19, 2022

michaelfig commented Feb 19, 2022

kriskowal commented Feb 19, 2022

michaelfig commented Feb 20, 2022

kriskowal commented Feb 20, 2022

michaelfig commented Feb 20, 2022

michaelfig commented Jan 28, 2022 •

edited by mhofman

Loading

turadg Jan 28, 2022 •

edited

Loading

mhofman Feb 3, 2022 •

edited

Loading

turadg commented Feb 19, 2022 •

edited

Loading