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

abort() method absent on promises returned by mw.Api #50

Closed
jwbth opened this issue Nov 1, 2024 · 9 comments · Fixed by #51
Closed

abort() method absent on promises returned by mw.Api #50

jwbth opened this issue Nov 1, 2024 · 9 comments · Fixed by #51

Comments

@jwbth
Copy link

jwbth commented Nov 1, 2024

Requests made with mw.Api have something other kinds of promises known to mankind don't have – the abort() method. It's added using constructions like .promise({ abort: xhr.abort }) in https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.api/index.js and allows to easily terminate network activity.

types-mediawiki doesn't know about it:
image

I fixed it locally by adding a type

type JQueryPromiseWithAbort<T> = JQuery.Promise<T> & Pick<JQuery.jqXHR, 'abort'>;

and replacing each and every JQuery.Promise<ApiResponse> in https://github.com/wikimedia-gadgets/types-mediawiki/blob/main/mw/Api.d.ts with JQueryPromiseWithAbort<ApiResponse>.

Can't vouch this is perfectly correct – please recheck.

@jwbth
Copy link
Author

jwbth commented Nov 1, 2024

And I guess you need to export this type somehow. Maybe add it to mw.Api namespace, e.g. add after WatchedPage

            type Promise<T extends ApiResponse = ApiResponse> = JQuery.Promise<T> & Pick<JQuery.jqXHR, 'abort'>;

so that it becomes mw.Api.Promise and exported like this? Users will then be able to define their own response types and supply them to the generic parameter.

There is the same story with the REST API, https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.api/rest.js. So you might have mw.Rest.Promise there:

            type Promise<T extends RestResponse = RestResponse> = JQuery.Promise<T> & Pick<JQuery.jqXHR, 'abort'>;

@Derugon
Copy link
Collaborator

Derugon commented Nov 2, 2024

Sounds good to me.

Some API methods only return a specific field from the response object (e.g. getToken, isCategory), returning something that is not an ApiResponse, but still providing an abort method, so I suggest we allow any type of return value:

type Promise<T = ApiResponse> = JQuery.Promise<T> & Pick<JQuery.jqXHR, 'abort'>;

@Derugon
Copy link
Collaborator

Derugon commented Nov 2, 2024

While we're on it, I suggest we also add types for the other response promise parameters.
If I'm not mistaken, on success base HTTP methods resolve with

(result: ApiResponse, jqXHR: JQuery.jqXHR<ApiResponse>)

and on error all methods fail with

(code: string, data: any, result: any, jqXHR: JQuery.jqXHR)

With PromiseBase we could use something like:

type Promise<T, U = never> = JQuery.PromiseBase<T,     string,       never,
                                                U,     any,          never,
                                                never, any,          never,
                                                never, JQuery.jqXHR, never> & Pick<JQuery.jqXHR, 'abort'>;

and

get(...): Api.Promise<ApiResponse, JQuery.jqXHR<ApiResponse>>;
getToken(...): Api.Promise<string>;
...

Note that the 4th type parameter of PromiseBase is used to type rest parameters, so the error callback signature becomes (t: string, u: any, v: any, ...s: JQuery.jqXHR[]) instead of (t: string, u: any, v: any, s: JQuery.jqXHR). I don't know how this could be prevented, without providing our own PromiseBase declaration.

@jwbth
Copy link
Author

jwbth commented Nov 2, 2024

on error all methods fail with

(code: string, data: any, result: any, jqXHR: JQuery.jqXHR)

As opposed to the resolving callback, rejection callbacks are trickier actually. There are 2 signatures (actually 3 if you count one that has 'OK response but empty result (check HTTP headers?)' as the second parameter, see https://github.com/wikimedia/mediawiki/blob/e0a61eccf6043d237876b37fddfdb1cb1b0ac3cb/resources/src/mediawiki.api/index.js#L302-L306), they are described at https://github.com/wikimedia/mediawiki/blob/e0a61eccf6043d237876b37fddfdb1cb1b0ac3cb/resources/src/mediawiki.api/index.js#L204-L232.

@Derugon
Copy link
Collaborator

Derugon commented Nov 7, 2024

So the 1st error parameter is always a string, then depending on it:

  • http also provides a HttpErrorData as below.
  • ok-but-empty also provides a message string, the empty response value ('' | null | undefined), and a jqXHR (encapsulating the empty response value),
  • any other code provides the ApiResponse twice and a jqXHR.

Based on the above promise type:

interface HttpErrorData {
    exception: string;
    textStatus: JQuery.Ajax.ErrorTextStatus;
    xhr: JQuery.jqXHR;
}

type EmptyResponseMessage = 'OK response but empty result (check HTTP headers?)';

type Promise<T = any, U = never, V = never, S = never>
    = JQuery.PromiseBase<T, string,                                                        never,
                         U, HttpErrorData | EmptyResponseMessage | ApiResponse,            never,
                         V, ApiResponse | '' | null | undefined,                           never,
                         S, JQuery.jqXHR<ApiResponse | '' | null | undefined> | undefined, never>
    & Pick<JQuery.jqXHR, 'abort'>;

@Derugon
Copy link
Collaborator

Derugon commented Nov 12, 2024

Some specific API methods provide different arguments on error (e.g. getUserInfo) or notify values (e.g. upload methods).
This means we may have to use as many type variables in mw.Api.Promise as with JQuery.PromiseBase (4 per set of arguments = 12). To make it easier to use, we could use tuples to specify type parameters, like:

interface HttpErrorData<T = any> {
    exception: string;
    textStatus: JQuery.Ajax.ErrorTextStatus;
    xhr: JQuery.jqXHR<T>;
}

type Promise<R extends Promise.Params = [ApiResponse, JQuery.jqXHR<ApiResponse>], // on success
             J extends Promise.Params = Promise.ErrorParams,                      // on fail
             N extends Promise.Params = []>                                       // on notify
    = JQuery.PromiseBase<R[0], J[0], N[0],
                         R[1], J[1], N[1],
                         R[2], J[2], N[2],
                         R[3], J[3], N[3]>
    & Pick<JQuery.jqXHR, 'abort'>;

namespace Promise {
    // JQuery.Promise uses 4 type variables T, U, V, and S.
    // Callbacks are typed with `(t: T, u: U, v: V, ...s: S[]) => void`.
    type Params = [any?, any?, any?, any?];

    // Arguments received by error callbacks when using base API methods.
    type ErrorParams =
        | ["http", HttpErrorData]
        | ["ok-but-empty", "OK response but empty result (check HTTP headers?)", "" | null | undefined, JQuery.jqXHR<"" | null | undefined>]
        | [string, ApiResponse, ApiResponse, JQuery.jqXHR<ApiResponse>];
}

A typical upload promise could then be specified as:

mw.Api.Promise<[ApiResponse],                           // on success
               [string, mw.Api.Promise.ErrorParams[1]], // on fail
               [number]>                                // on notify

Here is what it would look like with mw.Api.

@Derugon
Copy link
Collaborator

Derugon commented Nov 13, 2024

Some further changes to the above proposal:

  • maybe use the term callback "argument" instead of "parameter" to not confuse with API parameter types
  • mw.Rest uses similar promises, so we could use a common promise type (named mw.Api.PromiseBase below), for which we change the default reponse values (mw.Api.Promise and mw.Rest.Promise).
  • fix the type of unspecified parameters being inferred as undefined instead of never with the above implementation (since ([])[0] is undefined not never), by introducing a mapping type (Arg) to detect whether an array value is specified.

For the base promise format:

namespace Api {
    interface PromiseBase<TResolve extends ArgTuple, TReject extends ArgTuple, TNotify extends ArgTuple>
        extends JQuery.PromiseBase<Arg<TResolve, 0>, Arg<TReject, 0>, Arg<TNotify, 0>,
                                   Arg<TResolve, 1>, Arg<TReject, 1>, Arg<TNotify, 1>,
                                   Arg<TResolve, 2>, Arg<TReject, 2>, Arg<TNotify, 2>,
                                   Arg<TResolve, 3>, Arg<TReject, 3>, Arg<TNotify, 3>>,
                Pick<JQuery.jqXHR, "abort"> {}

    type ArgTuple = [any?, any?, any?, any?];

    // Extract an argument type from an ArgTuple. Works as following:
    // * If T is an empty tuple, argument type is `never`.
    // * If the argument to extract is the first one, get its type.
    // * Otherwise, remove the head (if it exists) of each array from the type union T
    //   then (tail-recursively) start over.
    type Arg<T extends ArgTuple, N extends number, TAcc extends never[] = []> =
        false extends (T extends [] ? true : false)
            ? TAcc["length"] extends N ? T[0] : Arg<Tail<T>, N, [...TAcc, never]>
            : never;
}

type Tail<T extends any[]> = T extends [] ? T : T extends [any?, ...infer R] ? R : T;

For mw.Rest:

namespace Rest {
    type Promise<TResolve extends Api.ArgTuple = [RestResponse, JQuery.jqXHR<RestResponse>],
                 TReject  extends Api.ArgTuple = RejectArgTuple,
                 TNotify  extends Api.ArgTuple = []>
        = Api.PromiseBase<TResolve, TReject, TNotify>;

    type RejectArgTuple =
        | ["http", HttpErrorData];
}

For mw.Api:

namespace Api {
    type Promise<TResolve extends ArgTuple = [ApiResponse, JQuery.jqXHR<ApiResponse>],
                 TReject  extends ArgTuple = RejectArgTuple,
                 TNotify  extends ArgTuple = []>
        = PromiseBase<TResolve, TReject, TNotify>;

    type RejectArgTuple =
        | Rest.RejectArgTuple
        | ["ok-but-empty", "OK response but empty result (check HTTP headers?)", "" | null | undefined, JQuery.jqXHR<"" | null | undefined>]
        | [string, ApiResponse, ApiResponse, JQuery.jqXHR<ApiResponse>];
}

Gist from previous comment is updated with that.

@Derugon
Copy link
Collaborator

Derugon commented Nov 14, 2024

The above implementation is in #51 (I still need to check if that could cause breakage, not sure about that).

@Derugon
Copy link
Collaborator

Derugon commented Nov 18, 2024

It seems mostly ok, apart from some special cases (listed in the associated PR description).
If no one has any other issue or comment on this and that's ok, I'll release it on the main branch before the end of week.

Derugon added a commit that referenced this issue Nov 22, 2024
`mediawiki.api`: Add custom `Promise` types to fix #50
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

Successfully merging a pull request may close this issue.

2 participants