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

Suggestion: improve types from summary response #16

Open
pierrebiver opened this issue Dec 18, 2023 · 4 comments
Open

Suggestion: improve types from summary response #16

pierrebiver opened this issue Dec 18, 2023 · 4 comments

Comments

@pierrebiver
Copy link

Hello,
I really like NL cloud and wanted to give you a quick feedback in regards to the typing issues I'm facing. I'm using the summarization function, and the return type is hard to use and a bit confusing:

 Promise<{
        status: number;
        statusText: string;
        data: {
            summary_text: string;
        };
    } | {
        status: number;
        statusText: string;
        data: {
            url: string;
        }
    }

TS is confused when I get the response back as it does not know if data contains an url or a summary_text. I'm not sure which case we get an url or a string, but would be nice to know in advance, otherwise a quick fix could look like this:

 Promise<{
        status: number;
        statusText: string;
        data: DataTypeText;
    } | {
        status: number;
        statusText: string;
        data: DataTypeUrl;
    }

and simply export the data type so we can type it when we get back the response ourself, right now I'm simply using // @ts-ignore which I would like to avoid.

@pierrebiver pierrebiver changed the title Suggestion: improve types from response Suggestion: improve types from summary response Dec 18, 2023
@juliensalinas
Copy link
Contributor

Hello @pierrebiver , thank you so much for this suggestion!
The challenge comes from the fact that, if you use this endpoint in asynchronous mode (https://docs.nlpcloud.com/#asynchronous-mode) you get a URL that you should poll later in order to get the result.
I totally agree with you, this is not satisfying...

I am not exactly sure I understand your suggestion above about data type export.
Could you please elaborate a bit?

Thank you so much, Looking forward to hearing from you!

Julien

@pierrebiver
Copy link
Author

pierrebiver commented Dec 19, 2023

Oh I didn’t know async mode would yield a different type, this is also a bit dangerous then, as I might plan to switch to async mode at some point. Would that make sense to return a different NLPClient based on whether async mode is enabled or not ?

Regarding the type, I meant doing something like this:

Export type DataTypeText = {
text: string; 
}

export type DatatypeUrl = {
URL: string;
}

Promise<{
        status: number;
        statusText: string;
        data: DataTypeText;
    } | {
        status: number;
        statusText: string;
        data: DataTypeUrl;
    }

In this case, I would be able to import the DataType I need and type my result. Does that makes sense ?

@pierrebiver
Copy link
Author

But I think having different types for the NLPClient based on whether the async mode is enabled or not would solve your problem and would probably be the best option. Otherwise switching to async mode would break my code without TS complaining about anything

@juliensalinas
Copy link
Contributor

Thank you Pierre for these valuable comments.
The fact that some endpoints have different signatures based on the async mode is definitely not 100% satisfactory, we agree.
Returning different clients might be an option indeed.
And thank you for the code snippet above. We are going to reviews your suggestions internally 👍🏻

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