Moved repo to strict: true and restructured tsconfig.jsons to use project references by mattpocock · Pull Request #567 · supermacro/neverthrow

mattpocock · 2024-08-20T08:03:04Z

No changeset needed, not a user-facing change.

changeset-bot · 2024-08-20T08:03:08Z

⚠️ No Changeset found

Latest commit: 276b3b6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

…and added strict: true

mattpocock · 2024-08-20T08:15:01Z

package.json

    "lint": "eslint ./src --ext .ts",
    "format": "prettier --write 'src/**/*.ts?(x)' && npm run lint -- --fix",
-    "typecheck": "tsc --noEmit",
+    "typecheck": "tsc -b",


Now, this typecheck command checks the entire repo, including the typecheck tests.

Doesn't this create artifacts, though? Shouldn't it be tsc -b --noEmit?

The whole intention of the typecheck script is to typecheck without generating artifacts.

No, noEmit has been moved to the config file - this is so that users running tsc without using the command don't accidentally generate artifacts.

mattpocock · 2024-08-20T08:15:58Z

tests/index.test.ts

      const example = ResultAsync.fromThrowable(
        () => Promise.reject('No!'),
-        (e) => new Error('Oops: ' + e)
+        (e) => new Error('Oops: ' + e),


Looks like prettier changes in this file.

mattpocock · 2024-08-20T08:16:18Z

tsconfig.json

+  "exclude": [],
+  "references": [
+    {
+      "path": "./tsconfig.tests.json"


This is now referenced in the root tsconfig, meaning both can be run simultaneously.

mattpocock · 2024-08-20T08:21:58Z

tsconfig.json

-      "declaration": true,
-      "baseUrl": "./src",
-      "lib": [
-        "dom",


Removed the DOM types, not needed here.

mattpocock · 2024-08-20T08:22:17Z

tsconfig.json

-      "sourceMap": false,
-      "noUnusedLocals": true,
-      "noUnusedParameters": true,
-      "strictNullChecks": true,


Slimmed down the tsconfig by flags inferred by other flags.

supermacro · 2024-09-06T15:12:26Z

.vscode/settings.json

I don't think neverthrow should include editor specific files. Please remove.

supermacro · 2024-09-06T15:14:52Z

src/result.ts

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    return ((...args: any[]) => {
      try {
        const result = fn(...args)
        return ok(result)
      } catch (e) {
        return err(errorFn ? errorFn(e) : e)
      }
-    }
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    }) as any


Can you explain the rationale for these as any changes made here and in other files?

Moving to strict: true revealed some errors that I didn't have time to debug. I suggest we 'as any' them now, and fix them in a followup PR.

The reason this errors and you have to as any it is because with this call signature you can in theory do something like:

const safeParse = fromThrowable<typeof JSON.parse, number>(JSON.parse); // ^ (text: string, reviver?: (this: any, key: string, value: any) => any) => Result<any, number>

So by allowing providing a specific generic type as E without providing an errorFn, you can get in trouble. You can then end up in a case where you try to pass e to the err in the catch block (in the second/false ternary case), but since the user might have specified a specific E (like number in the example above), you can't just give it the unknown (or any here I suppose)e.

The easiest fix might be to add some overloads to prevent using the function as I did above:

export function fromThrowable<Fn extends (...args: readonly any[]) => any>( fn: Fn ): (...args: Parameters<Fn>) => Result<ReturnType<Fn>, unknown>; export function fromThrowable<Fn extends (...args: readonly any[]) => any, E>( fn: Fn, errorFn: (e: unknown) => E, ): (...args: Parameters<Fn>) => Result<ReturnType<Fn>, E>; export function fromThrowable<Fn extends (...args: readonly any[]) => any, E>( fn: Fn, errorFn?: (e: unknown) => E, ): (...args: Parameters<Fn>) => Result<ReturnType<Fn>, E | unknown> { // implementation }

I don't know if it really makes any practical difference here, but I believe it would be more correct to do catch (e: unknown) to have e typed as unknown and not the default any.

This could of course happen in a different PR. And I suppose in theory changing the call signature as I proposed might be considered a breaking change. However, if someone is using the function as I did above they should stop doing as it probably gives you types that don't match the runtime types. So you could consider it a bug maybe 🤷

I created a separate PR for fixing the issue: #583 However, feel free to close that PR and include the changes here instead if that's better.

supermacro · 2024-09-06T15:15:47Z

package.json

    "lint": "eslint ./src --ext .ts",
    "format": "prettier --write 'src/**/*.ts?(x)' && npm run lint -- --fix",
-    "typecheck": "tsc --noEmit",
+    "typecheck": "tsc -b",


Doesn't this create artifacts, though? Shouldn't it be tsc -b --noEmit?

The whole intention of the typecheck script is to typecheck without generating artifacts.

supermacro · 2024-09-06T15:17:18Z

tsconfig.tests.json

+    "declaration": true,
+    "composite": true,
+    "outDir": "./tests/dist",
+    "noEmit": false


Shouldn't this be "noEmit": true?

Using project references means that you do need to generate declaration files. This acts as a kind of cache for TS, making things run a bit faster.

These files are outputted to ./tests/dist, as specified in outDir

This changes were first suggested by matt pock on: github.com/supermacro/pull/567/ The only difference is that we are not including the `strict: true` for now

Upgraded TS and moved to strict: true

9a30242

mattpocock requested review from m-shaka and supermacro as code owners August 20, 2024 08:03

Moved the tsconfig.test.json to be a reference of the root tsconfig, …

37ef88b

…and added strict: true

mattpocock commented Aug 20, 2024

View reviewed changes

Fixed CI by downgrading TS again

6771c47

mattpocock changed the title ~~Upgraded TS and moved to strict: true~~ Moved repo to strict: true and restructured tsconfig.jsons to use project references Aug 20, 2024

Reverted some tsconfig.json changes

276b3b6

mattpocock commented Aug 20, 2024

View reviewed changes

mattpocock marked this pull request as draft August 20, 2024 08:31

mattpocock mentioned this pull request Aug 20, 2024

Upgrade repo to modern tooling #572

Open

10 tasks

supermacro requested changes Sep 6, 2024

View reviewed changes

boyeln mentioned this pull request Sep 12, 2024

prevent passing a specific error type for .fromThrowable without providing an error mapping function #583

Open

ralacerda added a commit to ralacerda/neverfall that referenced this pull request Jul 20, 2025

chore: improve tsconfig files

d48140f

This changes were first suggested by matt pock on: github.com/supermacro/pull/567/ The only difference is that we are not including the `strict: true` for now

Conversation

mattpocock commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boyeln Sep 11, 2024 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattpocock commented Aug 20, 2024 •

edited

Loading

changeset-bot bot commented Aug 20, 2024 •

edited

Loading

boyeln Sep 11, 2024 •

edited

Loading