traverse1984 / oxide.ts

Rust's Option<T> and Result<T, E>, implemented for TypeScript.
MIT License
517 stars 19 forks source link

Working on next release - any suggestions? #15

Open traverse1984 opened 1 year ago

traverse1984 commented 1 year ago

Hi all,

I'm working on the next release version. I've made a few decisions so far and I'm interested in any thoughts or suggestions you may have (either on those decisions, or generally for the library).

Planned Changes

Considerations

a1ek5borodai commented 1 year ago

It would be good to have a built-in tap method for Option and Result to be able to handle logging:

const result = User.create({ name: 'John' })
      .tap((user) => this.logger.debug(user, 'User'))
      .map(this.usersRepository.createUser)
      .tap(() => this.logger.debug('User saved'))
      .and(Ok(undefined));

Also, it would be good to be able just to return Ok() without the need to explicitly pass undefined as an argument.

And it would be good to have a possibility to throw custom errors. So for example this code:

   expect(this: Result<T, E>, msg: string): T {
      if (this[T]) {
         return this[Val] as T;
      } else {
         throw new Error(msg);
      }
   }

will look more like this:

   expect(this: Result<T, E>, error: Error): T {
      if (this[T]) {
         return this[Val] as T;
      } else {
         throw error;
      }
   }

P.S. Thanks for your effort! Great library!

traverse1984 commented 1 year ago

Thanks for these suggestions. Will definitely take on board the empty Ok - great idea.

Expect will already be getting better in the next release, and I think your suggestion could be incorporated too - but I don't want to encourage constructing errors which are only thrown in some cases. I'm wondering whether lazy evaluation (() => Error) might be better.

Also interested in the tap method. What made you choose that name?

a1ek5borodai commented 1 year ago

I don't want to encourage constructing errors which are only thrown in some cases

Well, most probably in the case of the "expect" method, if an error will occur there in runtime, it is rather an exception. One may want to log this exception in the catch-all method or add an alarm depending on the exception type or do other things based on metadata. So in my mind, this will be a useful feature. You may provide options to the developer: to throw a base error with a custom message or to throw a custom error.

   expect(this: Result<T, E>, messageOrError: Error | string): T {
      if (this[T]) {
         return this[Val] as T;
      } else {
         if(typeof messageOrError === 'string') throw new Error(messageOrError )
         throw error;
      }
   }
a1ek5borodai commented 1 year ago

Also interested in the tap method. What made you choose that name?

Oh, that's from RxJS: https://rxjs.dev/api/operators/tap. So that's not my choice, but seems like a reasonable method name if you want to perform side effects in a pipe :)

traverse1984 commented 1 year ago

Well, most probably in the case of the "expect" method, if an error will occur there in runtime, it is rather an exception...

I also considered the case where some other method returns an error which gets held and (potentially) thrown at the expect point. I think it's worth adding alongside the other improvement.

In the case where you just want a custom error to be thrown you should not need to pre-construct "just in case". A signature that accepts a function could be useful here:

const x = y.expect((e) => new MyError(e));

In the next version the following would be functionally equivalent:

const x = y.mapErr((e) => new MyError(e)).expect();

I think I prefer the second syntax. So I'm trying to weigh that against changes to the expect signature so that things feel like they make sense.

I realise it's a very bit-picky point, but my aim is to keep things as clean as possible.

traverse1984 commented 1 year ago

Also interested in the tap method. What made you choose that name?

Oh, that's from RxJS: https://rxjs.dev/api/operators/tap. So that's not my choice, but seems like a reasonable method name if you want to perform side effects in a pipe :)

In a pipe... of course. Makes sense really, thanks!

sahellebusch commented 1 year ago

First, love the library. Great work here!

Small nitpick suggestion: auto-imports pull in oxide.ts/dist which doesn't compile.

Error: Package subpath './dist' is not defined by "exports" in /usr/src/app/node_modules/oxide.ts/package.json
api_1       |     at new NodeError (node:internal/errors:399:5)
api_1       |     at exportsNotFound (node:internal/modules/esm/resolve:361:10)
api_1       |     at packageExportsResolve (node:internal/modules/esm/resolve:697:9)
api_1       |     at resolveExports (node:internal/modules/cjs/loader:565:36)
api_1       |     at Function.Module._findPath (node:internal/modules/cjs/loader:634:31)
api_1       |     at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1061:27)
api_1       |     at Function.Module._load (node:internal/modules/cjs/loader:920:27)
api_1       |     at Module.require (node:internal/modules/cjs/loader:1141:19)
api_1       |     at require (node:internal/modules/cjs/helpers:110:18)
api_1       |     at Object.<anonymous> (/usr/src/app/src/lib/mapper.types.ts:1:1)
janydoe commented 1 year ago

I want to see something like isOption, isResult functions

janydoe commented 1 year ago

Also, it would be great to see inspect and inspectErr implementations

Akiyamka commented 1 year ago

Wolud be cool if Result.unwrap() throw error with message from Err instead of generic Failed to unwrap Result (found Err).

I found way to achieve this, but it pretty verbose

const env = match(result, {
  Ok: (val) => val,
  Err: (err) => {
    throw Error(err)
   }
})
kiranpradeep commented 10 months ago

I like to take advantage of narrowing and use result without unwrapping like below.

const res = Result.from(1);
if (isOk(res)) {
   // const val = res.unwrap();
   console.log(res.val);
   // more processing directly with res.val instead of unwrapped val
}
DeluxeOwl commented 6 months ago

Wolud be cool if Result.unwrap() throw error with message from Err instead of generic Failed to unwrap Result (found Err).

I found way to achieve this, but it pretty verbose

const env = match(result, {
  Ok: (val) => val,
  Err: (err) => {
    throw Error(err)
   }
})

I was really surprised to see that this wasn't the case

DeluxeOwl commented 6 months ago

I'd really like some serialization and deserialization options:

import { Option, Some } from "oxide.ts";

const foo: Option<string> = Some("hello");

// doesnt print anything
console.log(JSON.stringify(foo, null, 2));
korniychuk commented 2 hours ago

I'm not sure that it's possible to implement the full LSP support for it ... However, I'm thinking about the idea of extending TS syntax, not just writing types. For example, implement some kind of ? operator for Option/Result propagation like in Rust.