mobily / ts-belt

🔧 Fast, modern, and practical utility library for FP in TypeScript.
https://mobily.github.io/ts-belt
MIT License
1.08k stars 30 forks source link

Bad Typing Ok / Error #98

Open mattaiod opened 7 months ago

mattaiod commented 7 months ago

Hello everyone,

=> PROBLEM: __: 'Ok' doesn't exists in the Ok object in javascript, but exists in the typescript type. => REASON: the type lies about the real js object => PROPOSED SOLUTION: add __: 'Ok' to the Ok object js

I found in the source code that Ok and Error are incorrectly typed. I assume this is intentional, but it's a problem because the type lies about the actual object. For example, this is a problem in my use of the function stringify in the typia lib.

in node-modules of my project I found:

file: ts-belt/dist/types/Result/index.d.ts export declare const Ok: (value: T) => Ok;

export declare type Ok = { readonly TAG: 0; readonly _0: T; } & { __: 'Ok'; };

file: ts-belt/dist/Result.js const Ok = r => ({ TAG: 0, _0: r });

Like you can see, the type lies about the real js object ...

What do you think about that ?

Thank you and have a nice day :)

JUSTIVE commented 7 months ago

+1 for this. with ts-pattern, I always use { TAG:0 } instead of { __:'OK' }, which leads poor readability.

mattaiod commented 7 months ago

to solve the problem I mentioned i have created :

export const Left = <T>(error: T): Error<T> => {
  return {
    TAG: 1,
    _0: "data",
    __: "Error",
  };
};

export const Right = <T>(value: T): Ok<T> => {
  return {
    TAG: 0,
    _0: "data",
    __: "Ok",
  };
};

=> so object and type are identical

and in my eslint I have forbidden the use of Ok and Error function :

"no-restricted-syntax": [
      "error",
      {
        "selector": "CallExpression[callee.name='Error']",
        "message": "Error is deprecated, use Left instead because it complies the full type Error of the lib ts-belt.  "
      },
      {
        "selector": "CallExpression[callee.name='Ok']",
        "message": "Ok is deprecated, use Right instead because it complies the full type Ok of the lib ts-belt.  "
      },
]
mattaiod commented 3 months ago

@mobily I would like to draw your attention to this very important issue.

Since this library is based on typescript and moreover on functional programming, it is very important that the types and implementation match.

thank you in advance

JUSTIVE commented 3 months ago

those __:'Ok' | 'Error' s are not belt(the core of this library)'s part. It's added by author @mobily's. It could be cumbersome to match all belt functions to work with __ fields. I'd still recommend using TAG : 0 | 1 field, which could be easily reminded from exit code. Rescript's internal functions use TAG field.

mattaiod commented 3 months ago

@JUSTIVE Okay, I get the point!

The only potential problem would be if someone used a library or coded something with the same structure, then there would be a conflict.

I mean anything that uses a structure with : TAG : 0 | 1

The best way to avoid this would be to have a key like: BELT_TAGinstead of TAG or an other key showing this structure is from ts-belt

-> Of course, I'm aware that this type of conflict may be rare, but I like to use the safest code :)

JUSTIVE commented 3 months ago

I see the concerns you have. But I think It's quite a rare to have such type, names. would be great to adopt your suggestion. I'll make a PR for that, but It'll break codes worked with previous versions.