[New] add types by ljharb · Pull Request #603 · tape-testing/tape

ljharb · 2024-03-02T22:46:49Z

Normally I'd just push this commit up and publish it, but since @types/tape already exists, I thought this was worth a PR so others can double check me.

Raynos · 2024-03-04T14:54:42Z

Are these hand written or generated from JSDoc ? It's unclear to me. Either way looks reasonable to me.

ljharb · 2024-03-04T15:18:11Z

Hand-written.

JoshuaKGoldberg

Cool to see this addition - thanks for mentioning it!

I can take a deeper look if the tsc having a lot of errors already thing isn't a blocker.

ljharb · 2024-03-11T18:43:45Z

Thanks, I've updated the PR and gotten it down to 4 errors (i swear it was at zero when I put up the PR :-p)

I'd love to get suggestions for those.

JoshuaKGoldberg · 2024-03-23T18:29:42Z

Oop, sorry for taking so long - I'd added this tab on my phone and forgot about it 😅. Looking now!

it down to 4 errors

On the latest commit I get no errors. So, nicely done! 😄

JoshuaKGoldberg

This looks great!

Left some suggestions, but nothing showing anything wrong. Nicely done! 👏

JoshuaKGoldberg · 2024-03-23T18:37:27Z

You might consider also porting in the types tests from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/e1386bb7838c35504edfdb12876bf0d156d84a2a/types/tap/tap-tests.ts. Since the .d.ts is being hand-written rather than auto-generated from source files, it wouldn't be unheard of for it to fall out of sync.

if it were out of sync, wouldn't tsc fail?

Not in all cases. If you, say, added an extra optional parameter somewhere but never used it, tsc wouldn't think to fail on it.

JoshuaKGoldberg · 2024-03-23T18:43:58Z

 		expected = undefined;
 	}

+	/** @type {undefined | { error: unknown | Error }} */


error: unknown | Error is equivalent to error: unknown. unknown is a top type indicating it could be anything in the world, including Error. plugin:@typescript-eslint/no-redundant-type-constituents

The code later on just checks if (caught.error && typeof caught.error === 'object'), not instanceof Error or an equivalent. So if a caught.error is a non-Error object, the assumption it could be an Error would be wrong.

Can caught.error be something other than Error? If so, what?

technically line 897 doesn't ensure it's an Error, but if that branch is entered, then it's so Error-like as to not make a difference. there doesn't seem to be any way i can do a type assertion without runtime code :-/

JoshuaKGoldberg · 2024-03-23T18:50:13Z

+				// TS TODO: `caught.error` and `expected` should both be `object` here
+				passed = every(keys, /** @type {(key: PropertyKey) => boolean} */ function (key) {


Unfortunately, TypeScript doesn't have a way of being informed that every's callbacks should keep narrowing. A type assertion would be the appropriate thing to do here instead of // @ts-expect-error to avoid changing runtime code.

A small runtime change could be to store error and expected as variables after narrowing them.

Alternately, the next version of TypeScript might be nicer about preserving narrowings - I haven't tried.

i'm using next, so it doesn't help me here. this isn't TS-expect-error tho; it's just a TODO comment

I meant the // @ts-expect-errors later.

socket-security · 2025-02-10T19:32:33Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	npm/eslint@8.57.1 ⏵ 10.4.1	⁺²			⁺⁴⁵

View full report

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (6)

test/intercept.js (3)
19-19: Add clarification to the error suppression comment

The comment // @ts-expect-error TS sucks with concat could be more professional and descriptive of the actual issue. Consider changing it to explain the specific type issue with array concatenation.
-// @ts-expect-error TS sucks with concat
+// @ts-expect-error TypeScript has issues with array concatenation of heterogeneous types
152-153: Consider using a more specific type for object properties

While Record<PropertyKey, unknown> works, you might consider using a more specific type that describes the expected structure of the object, including the foo property and the inspect method.
-/** @type {Record<PropertyKey, unknown>} */
+/** @type {{ foo: unknown, inspect(): string} & Record<PropertyKey, unknown>} */
245-246: Consider a more specific type for the 'val' variable

Using unknown is safer than any, but since this variable will only store values set to the setter property, you could consider using a more specific type or adding a comment explaining its purpose.
-/** @type {unknown} */
+/**
+ * @type {unknown}
+ * Stores the value set by the setter property
+ */
index.d.ts (1)

6-20: Consider unifying harnessFunction overloads.

Multiple overloads can be harder to maintain. It might be tidier to provide one or two broader overloads and make parameters optional, if feasible.

lib/test.js (2)

59-80: Consider simplifying argument parsing in getTestArgs.

Using multiple argument types in a loop is functional, but you could consider a single parameter object or other patterns for more straightforward type definitions.

120-125: Offer assistance with @ts-expect-error usage.

There's a // @ts-expect-error hint at line 122. If you need help refining the type so this directive can be removed or replaced with a safer assertion, let me know.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 035877d and c75669b.

⛔ Files ignored due to path filters (3)

.github/workflows/node-aught.yml is excluded by !**/*.yml
package.json is excluded by !**/*.json
tsconfig.json is excluded by !**/*.json

📒 Files selected for processing (81)

.eslintrc (1 hunks)
bin/import-or-require.js (1 hunks)
bin/tape (2 hunks)
example/array.js (2 hunks)
example/fail.js (2 hunks)
example/nested.js (2 hunks)
example/nested_fail.js (2 hunks)
example/not_enough_fail.js (2 hunks)
example/too_many_fail.js (2 hunks)
index.d.ts (1 hunks)
index.js (8 hunks)
lib/default_stream.d.ts (1 hunks)
lib/default_stream.js (2 hunks)
lib/isObject.js (1 hunks)
lib/results.d.ts (1 hunks)
lib/results.js (9 hunks)
lib/test.d.ts (1 hunks)
lib/test.js (42 hunks)
test/anonymous-fn/test-wrapper.js (1 hunks)
test/array.js (2 hunks)
test/assertion.js (4 hunks)
test/async-await.js (2 hunks)
test/async-await/async-bug.js (3 hunks)
test/async-await/async4.js (1 hunks)
test/async-await/async5.js (3 hunks)
test/capture.js (6 hunks)
test/captureFn.js (3 hunks)
test/comment.js (4 hunks)
test/common.js (6 hunks)
test/deep-equal-failure.js (3 hunks)
test/deep.js (1 hunks)
test/default_stream.js (1 hunks)
test/double_end.js (2 hunks)
test/end-as-callback.js (2 hunks)
test/exit.js (7 hunks)
test/exit/fail.js (2 hunks)
test/exit/ok.js (2 hunks)
test/exit/too_few.js (2 hunks)
test/fail.js (2 hunks)
test/ignore-pattern.js (3 hunks)
test/ignore_from_gitignore.js (4 hunks)
test/import.js (4 hunks)
test/import/mjs-a.mjs (1 hunks)
test/import/mjs-b.mjs (1 hunks)
test/import/mjs-c.mjs (1 hunks)
test/import/mjs-d.mjs (1 hunks)
test/import/mjs-e.mjs (1 hunks)
test/import/mjs-f.mjs (1 hunks)
test/import/mjs-g.mjs (1 hunks)
test/import/mjs-h.mjs (1 hunks)
test/import/package_type/package-a.js (1 hunks)
test/import/package_type/package-b.js (1 hunks)
test/import/package_type/package-c.js (1 hunks)
test/intercept.js (11 hunks)
test/match.js (2 hunks)
test/max_listeners.js (1 hunks)
test/nested.js (2 hunks)
test/no_only.js (3 hunks)
test/not-deep-equal-failure.js (3 hunks)
test/objectMode.js (2 hunks)
test/objectModeWithComment.js (2 hunks)
test/onFailure.js (1 hunks)
test/only.js (1 hunks)
test/promises/fail.js (1 hunks)
test/promises/subTests.js (1 hunks)
test/require.js (3 hunks)
test/require/a.js (1 hunks)
test/require/b.js (1 hunks)
test/require/test-a.js (1 hunks)
test/require/test-b.js (1 hunks)
test/skip.js (1 hunks)
test/stackTrace.js (15 hunks)
test/strict.js (1 hunks)
test/subtest_and_async.js (1 hunks)
test/teardown.js (5 hunks)
test/throws.js (6 hunks)
test/timeoutAfter.js (1 hunks)
test/todo.js (1 hunks)
test/too_many.js (2 hunks)
types/ecstatic/index.d.ts (1 hunks)
types/tap-parser/index.d.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (73)

test/require/test-a.js
test/async-await/async4.js
test/promises/fail.js
test/promises/subTests.js
test/import/package_type/package-c.js
test/import/mjs-a.mjs
test/require/test-b.js
test/import/mjs-h.mjs
test/anonymous-fn/test-wrapper.js
test/import/package_type/package-a.js
test/import/mjs-g.mjs
test/skip.js
test/subtest_and_async.js
test/only.js
.eslintrc
test/import/mjs-b.mjs
test/import/package_type/package-b.js
test/require/b.js
test/ignore-pattern.js
test/timeoutAfter.js
bin/import-or-require.js
test/exit.js
test/import/mjs-c.mjs
lib/default_stream.d.ts
test/onFailure.js
test/async-await/async5.js
test/deep.js
test/captureFn.js
test/require.js
test/require/a.js
test/import/mjs-e.mjs
types/ecstatic/index.d.ts
test/too_many.js
test/default_stream.js
test/async-await.js
test/import/mjs-f.mjs
test/max_listeners.js
test/todo.js
test/exit/ok.js
test/capture.js
test/ignore_from_gitignore.js
test/array.js
test/exit/fail.js
example/array.js
bin/tape
test/teardown.js
lib/default_stream.js
example/not_enough_fail.js
test/nested.js
example/too_many_fail.js
test/import/mjs-d.mjs
test/strict.js
example/nested_fail.js
test/double_end.js
example/fail.js
test/throws.js
test/match.js
test/exit/too_few.js
test/objectMode.js
test/common.js
test/deep-equal-failure.js
test/assertion.js
example/nested.js
test/fail.js
test/objectModeWithComment.js
test/stackTrace.js
test/end-as-callback.js
test/not-deep-equal-failure.js
types/tap-parser/index.d.ts
test/async-await/async-bug.js
test/import.js
lib/results.js
test/comment.js

🧰 Additional context used

🧠 Learnings (6)

lib/results.d.ts (1)

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/results.d.ts:18-20
Timestamp: 2024-11-27T00:54:06.607Z
Learning: Do not suggest adding the `private` modifier to properties and methods in TypeScript code, as `private` is not part of standard JavaScript. Avoid recommending the use of the `private` keyword in code reviews for TypeScript projects.

index.d.ts (2)

Learnt from: ljharb
PR: tape-testing/tape#603
File: index.d.ts:68-70
Timestamp: 2024-11-27T00:58:49.749Z
Learning: In a `.d.ts` file, it's acceptable to declare constants of types imported with `import type`, because type and value spaces are separate.

Learnt from: ljharb
PR: tape-testing/tape#603
File: index.js:142-143
Timestamp: 2025-02-06T20:57:58.411Z
Learning: In tape's index.js, the test function returned by createHarness implements all Harness type properties: only, close, createStream, onFinish, onFailure, _tests, _exitCode, and _results. The run property is optional.

lib/test.js (1)

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.js:0-0
Timestamp: 2024-11-12T06:00:40.803Z
Learning: The project `tape-testing/tape` uses ES3 syntax only and does not use TypeScript as a language. However, it employs the TypeScript Compiler (tsc) for type checking JavaScript files. This distinction is crucial for understanding the project's approach to handling types and syntax.

lib/test.d.ts (7)

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.d.ts:14-20
Timestamp: 2024-11-12T06:00:40.803Z
Learning: When suggesting changes to constructor overloads in TypeScript, ensure that all possible argument combinations are covered, including the option for a single callback argument.

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.d.ts:198-198
Timestamp: 2024-11-27T00:56:04.598Z
Learning: In this codebase, using `{}` as a type for the `receiver` property is intentional to allow primitives as receivers because `unknown` is invalid in this context.

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.d.ts:112-112
Timestamp: 2024-11-27T00:57:05.043Z
Learning: In `lib/test.d.ts`, for the `throws` method's `exceptionExpected` parameter, using `Function` as a type is correct and should not be replaced with `ErrorConstructor`, as it avoids introducing bugs.

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.d.ts:126-126
Timestamp: 2024-11-27T00:56:43.820Z
Learning: In the TypeScript declarations in `lib/test.d.ts`, for the `throws` method in the `Test` class, the `exceptionExpected` parameter intentionally uses the `Function` type to accept any function-like value. Replacing it with `ErrorConstructor` is not appropriate in this context.

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.d.ts:192-192
Timestamp: 2024-11-27T00:56:52.796Z
Learning: In `lib/test.d.ts`, it's acceptable to use `{}` as the type for the `receiver` property.

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.d.ts:172-172
Timestamp: 2025-02-06T18:06:43.090Z
Learning: In TypeScript, trailing commas after rest parameters are allowed specifically in .d.ts files (ambient contexts) as an exception to the normal syntax rules, even though they would trigger TS1013 errors in regular TypeScript files.

Learnt from: ljharb
PR: tape-testing/tape#603
File: lib/test.d.ts:172-172
Timestamp: 2025-02-06T18:06:43.090Z
Learning: Trailing commas are allowed in rest parameters and other locations in TypeScript declaration files (.d.ts), unlike JavaScript files.

index.js (3)

Learnt from: ljharb
PR: tape-testing/tape#603
File: index.js:147-147
Timestamp: 2024-11-12T06:00:40.803Z
Learning: Comments in the codebase should focus on explaining "why" or providing history/motivation, rather than detailing "what" the code does, as per ljharb's guidance.

Learnt from: ljharb
PR: tape-testing/tape#603
File: index.js:142-143
Timestamp: 2025-02-06T20:57:58.411Z
Learning: In tape's index.js, the test function returned by createHarness implements all Harness type properties: only, close, createStream, onFinish, onFailure, _tests, _exitCode, and _results. The run property is optional.

Learnt from: ljharb
PR: tape-testing/tape#603
File: index.js:211-224
Timestamp: 2025-02-10T20:24:25.187Z
Learning: In the tape module, module.exports is a function that must be mutated with additional properties. Using an object literal or Object.assign would either break functionality or add unnecessary dependencies.

test/no_only.js (1)

Learnt from: ljharb
PR: tape-testing/tape#603
File: test/no_only.js:47-47
Timestamp: 2025-03-03T23:09:11.191Z
Learning: The tape-testing repository uses ES3 syntax, which means modern JavaScript features like optional chaining (?.) cannot be used.

🪛 Biome (1.9.4)

index.d.ts

[error] 85-85: void is confusing inside a union type.

Unsafe fix: Use undefined instead.