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

Bad Typing Ok / Error #98

Open
mattaiod opened this issue Jan 20, 2024 · 6 comments
Open

Bad Typing Ok / Error #98

mattaiod opened this issue Jan 20, 2024 · 6 comments

Comments

@mattaiod
Copy link

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
Copy link
Sponsor

JUSTIVE commented Jan 21, 2024

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

@mattaiod
Copy link
Author

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
Copy link
Author

@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
Copy link
Sponsor

JUSTIVE commented May 22, 2024

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
Copy link
Author

@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_TAG instead 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
Copy link
Sponsor

JUSTIVE commented May 22, 2024

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.

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

No branches or pull requests

2 participants