supermacro / neverthrow

Type-Safe Errors for JS & TypeScript
MIT License
4.03k stars 84 forks source link

feat: add `ResultValue` and `ResultError` helper types #591

Open lucaschultz opened 1 month ago

lucaschultz commented 1 month ago

Hi,

I've been enjoying neverthrow for a while now. Since I try to rely on type inference as much as possible, I used some version of the types added in this PR in each project I used neverthrow in. Basically, they allow using this pattern:

function createSomething(): Result<{ name: string }, Error> {
  return ok({ name: 'John' })
}

function useSomething(something: ResultValue<ReturnType<typeof createSomething>>) {
  console.log(something.name)
}

Even better would be a version that would allow something like:

function createSomething() {
  if (somethingRandom()) {
    return err(new Error())
  }

  return ok({ name: 'John' })
}

function useSomething(something: ResultValue<ReturnType<typeof createSomething>>) {
  console.log(something.name)
}

But I couldn't figure out a straightforward way to achieve this (maybe @mattpocock could help). I would love to see these added, as not every TS developer knows how to use TS features like conditional types and infer, and having them be part of the library could encourage more developers to rely on type inference.

changeset-bot[bot] commented 1 month ago

⚠️ No Changeset found

Latest commit: 73c78d069287f087a7591566db4ed88b41c4da01

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

lucaschultz commented 4 weeks ago

It seems like a fine addition though I haven't found a need for it myself.

Would you consider naming them OkType and ErrType? It seems clearer to me. Not married to either.

Splitting each into its own file could be a bit scattered, it might be easier to put them both in a common types.ts.

I actually realized the codebase already had utility types like these. So I moved them into the respective result.ts and result-async.ts files and exported them. However, I'm facing an issue where this creates a circular dependency, which yields a warning during the build. I think this might be fine since the circular dependency is only on the type level? If I move the files to a types.ts file and export them from index.ts, which would not create a circular dependency, I get an error when running the type tests:

❯ npm run test-types

> neverthrow@8.0.0 test-types
> tsc --noEmit -p ./tests/tsconfig.tests.json

src/types.ts:1:24 - error TS2307: Cannot find module 'result' or its corresponding type declarations.

1 import { Result } from 'result'
                         ~~~~~~~~

src/types.ts:2:29 - error TS2307: Cannot find module 'result-async' or its corresponding type declarations.

2 import { ResultAsync } from 'result-async'
                              ~~~~~~~~~~~~~~

Found 2 errors in the same file, starting at: src/types.ts:1

Maybe @supermacro or @m-shaka can help? 😌

macksal commented 4 weeks ago

@lucaschultz

For the circular dependency problem - you can likely sidestep it using import type { foo } from "bar"; which is only used during type checking but doesn't build an import statement into the output. Therefore no issue at runtime but I think it will stop the build warning too.

The other import problem seems to be an issue with using absolute imports: from "result" instead of from "./result". This actually works inside the library because of the tsconfig's baseUrl: "./src" property (though I'm not sure it works after build, it may also depend on the build tools if not using tsc). You can likely fix by changing them to relative imports.

I did also notice that tsconfig.tests.json sets baseUrl incorrectly, it should be ../src instead of ./src. Fixing that would probably get it working too. For context, the tests aren't running against a built version of neverthrow, they directly run against the src files, and so the module resolution is whatever is configured by tsconfig.tests.json and not what's in the root tsconfig. If you set baseUrl: ../src, it will be the same as in the root tsconfig so absolute imports would resolve the same way.