more explicit type exports #1307
Conversation
packages/captp/jsconfig.json
Outdated
| "downlevelIteration": true, | ||
| "strictNullChecks": true, | ||
| "moduleResolution": "node", | ||
| "types": [] |
There was a problem hiding this comment.
Curious what motivated this. It seems about right, but I would not have thought of changing this.
There was a problem hiding this comment.
Without it there are errors from node_modules/events,
../../node_modules/events/events.js:53:3 - error TS2323: Cannot redeclare exported variable 'defaultMaxListeners'.
53 Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
54 enumerable: true,
~~~~~~~~~~~~~~~~~~~~~
...
64 }
~~~~~
65 });
~~~~
../../node_modules/events/events.js:67:3 - error TS2323: Cannot redeclare exported variable 'defaultMaxListeners'.
67 EventEmitter.defaultMaxListeners = defaultMaxListeners;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../node_modules/events/events.js:165:11 - error TS2339: Property 'context' does not exist on type 'Error'.
165 err.context = er;
~~~~~~~
I think because https://www.npmjs.com/package/@types/events is very stale. Even https://www.npmjs.com/package/events targets Node 11 API.
I looked into removing it but most ses-integration-test builders depend on it.
packages/promise-kit/package.json
Outdated
| @@ -40,6 +43,7 @@ | |||
| "devDependencies": { | |||
| "@endo/eslint-config": "^0.5.1", | |||
| "@endo/ses-ava": "^0.2.33", | |||
| "@types/node": ">=11.0", | |||
There was a problem hiding this comment.
I'd be ok with bumping this up to 14.0
There was a problem hiding this comment.
done. and since that still resolves to 14 that was available in yarn.lock, I also did a yarn upgrade.
| /** @type {typeof rawTest} */ | ||
| // @ts-expect-error cast |
There was a problem hiding this comment.
I wish we could type wrapTest correctly to avoid this.
| @@ -58,7 +60,7 @@ function testRacePromise(t, candidate) { | |||
| const result = Promise.resolve(candidate); | |||
|
|
|||
| bothResults = Promise.allSettled([raced, result]); | |||
| fr.register(raced, collected.resolve); | |||
| fr.register(raced, () => collected.resolve(undefined)); | |||
There was a problem hiding this comment.
Nothing to do with this really, but it's one of my biggest nit with tsc that () => void is incompatible with () => any. If it wasn't a test, I'd have asked for correcting the type of collected to PromiseKit<void> instead as I dislike changing runtime for types.
| @@ -30,6 +30,9 @@ | |||
| }, | |||
| "scripts": { | |||
| "build": "exit 0", | |||
| "clean": "tsc --build jsconfig.build.json --clean", | |||
There was a problem hiding this comment.
This is fine, but it may be good to manually verify that this works. The last Endo publish didn’t clean up after itself and I’m suspicious. I did not look deeply into why. It may be a problem higher up the foodchain, like lerna not running postpack.
There was a problem hiding this comment.
when I ran yarn clean it didn't remove the generated files. blocker? seems out of scope to me.
There was a problem hiding this comment.
Let’s remove the ineffective script and proceed. The work around for now is an additional step in the release process that involves nuking files that are not checked in. That carries some risk that I’ll bear going forward, until we find a better way.
There was a problem hiding this comment.
I haven't removed it yet because, from what I've read, it sometimes works. It's just very cautious to never delete if some files have been touched. If this is a blocker lemme know
turadg
force-pushed
the
ta/promise-kit-types
branch
from
9cd2ddd
to
5d9792f
Compare
npm WARN prepublish-on-install As of npm@5, `prepublish` scripts are deprecated. npm WARN prepublish-on-install Use `prepare` for build steps and `prepublishOnly` for upload-only.
turadg
force-pushed
the
ta/promise-kit-types
branch
from
73ad7dd
to
9d3e266
Compare
Progress on #1254