Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ResultValue and ResultError helper types #591

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lucaschultz
Copy link

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.

Copy link

changeset-bot bot commented Oct 9, 2024

⚠️ No Changeset found

Latest commit: 73c78d0

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

Copy link
Contributor

@macksal macksal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/result-error.ts Outdated Show resolved Hide resolved
@lucaschultz
Copy link
Author

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

> [email protected] 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
Copy link
Contributor

macksal commented Oct 24, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants