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

Error type is missing the source property #141

Open
AndyOGo opened this issue Jun 9, 2021 · 5 comments
Open

Error type is missing the source property #141

AndyOGo opened this issue Jun 9, 2021 · 5 comments
Labels
enhancement Suggests, requests, or implements a feature or enhancement

Comments

@AndyOGo
Copy link

AndyOGo commented Jun 9, 2021

RSocket error frames are turning into standard Error() objects, and expanded by a source property.
The Error type does not reflect that fact.

Expected Behavior

All errors should be of a custom error type, if applicable.
And type guards could be useful too.

interface RSocketErrorSource {
  code: number;
  explanation: string;
  message: string;
}

interface RSocketError extends Error {
  source: RSocketErrorSource;
}
function isRSocketError(value: any): value is RSocketError {
  return Object.prototype.toString.call(value) === "[object Error]" && isRSocketErrorSource(value.source);
}

function isRSocketErrorSource(value: any): value is RSocketErrorSource {
  return source && source?.code && source?.explanation && source?.message;
}

Actual Behavior

Insufficient Error type is used, this prohibits auto-completion and discovery.

Affected code examples:

  • export function createErrorFromFrame(frame: ErrorFrame): Error {
    const {code, message} = frame;
    const explanation = getErrorCodeExplanation(code);
    const error = new Error(
    sprintf(
    'RSocket error %s (%s): %s. See error `source` property for details.',
    toHex(code),
    explanation,
    message,
    ),
    );
    (error: any).source = {
    code,
    explanation,
    message,
    };
    return error;
    }

Possible Solution

Please see expected above, it shows an interface.

Your Environment

  • RSocket version(s) used: 0.0.25
@AndyOGo
Copy link
Author

AndyOGo commented Jun 10, 2021

TS types would also need an update, like:
https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/rsocket-types

@SerCeMan
Copy link
Member

Hey, @AndyOGo! Could you elaborate on what the desired change is? While createErrorFromFrame creates an error that has a type that can be described as RSocketError, the other references types, e.g. Flowable.js can accept any error, not necessarily an error having the source property.

@AndyOGo
Copy link
Author

AndyOGo commented Jun 16, 2021

Hi @SerCeMan ! Thank you for your reply.

That is my point, createErrorFromFrame returns type Error.
This typing is wrong, what it really returns is an extended error object, therefore the type should be RSocketError

export function createErrorFromFrame(frame: ErrorFrame): Error {

It should change to:

export function createErrorFromFrame(frame: ErrorFrame): RSocketError { 

You are right, other refs can accept any error.

For these cases a type guard would be great, like:

function isRSocketError(value: any): value is RSocketError {
  return Object.prototype.toString.call(value) === "[object Error]" && isRSocketErrorSource(value.source);
}

function isRSocketErrorSource(value: any): value is RSocketErrorSource {
  return source && source?.code && source?.explanation && source?.message;
}

@viglucci
Copy link
Member

viglucci commented Mar 6, 2022

Hey @AndyOGo , @SerCeMan,

Is my understanding correct that this mostly causes issues with developer experience related to code completion and IDE functionality? If so, I would propose that this is like a "won't fix", since we are primarily focused on #158, which will aim to provide better TypeScript support. Please let me know if I am misunderstanding.

@viglucci viglucci added the enhancement Suggests, requests, or implements a feature or enhancement label Mar 6, 2022
@AndyOGo
Copy link
Author

AndyOGo commented Mar 7, 2022

@viglucci
Our use case was mainly for error reporting and is production critical.

Anyway you are right too, it also enhances code completion.
#158 sounds great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Suggests, requests, or implements a feature or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants