WIP: fix: resolve typescript 3.7 errors

[EricCrosson]

Summary: this request improves upon type-safety for JavaScript users and can iron-out a currently-undefined behavior, but I could use help deciding on the proper behavior to take.

This merge is identical in intent to #113 but takes a different approach.

The differences can be summed up as follows:

• preserve exporting only the type (now interface) of classes Event, ErrorEvent, and CloseEvent (the classes themselves are not exported)
• parse input from program boundaries truthfully

The latter change was necessary because

if (url.then) {
    return url;
}

in _getNextUrl is marked as an error by the typescript compiler as it is a redundant check since url's type-signature string | Promise<string>.

However, the URL provider test indicates the need for type-safety for the use-case where the calling code is JavaScript, not TypeScript, and thus we cannot be sure that a) url is really a string | Promise<string> and b) that the Promise will resolve with a string.

I have added run-time type-checking code in the _parseUrl function so that we can always rely on our type signature, but since it is impossible to know what type a Promise will resolve with until it resolves, the type of the Promise returned by _getNextUrl was necessarily loosened to Promise<unknown> and thus some of this type-checking must be deferred until the Promise resolves in _connect.

Here we see the fruits of this exercise as it highlights an inconsistent state with undefined behavior that is possible to reach with the current (published) version of reconnecting-websocket: the url returned by _getNextUrl may not be a string at all.

The URL provider test ensures that ws._getNextUrl(123) and ws._getNextUrl(123) both error, but _getNextUrl does not handle the case where its argument is a non-string-returning Promise, e.g. ws._getNextUrl(() => Promise.resolve(123), which may certainly come from calling-code written in JavaScript.

The current behavior when _getNextUrl resolves a non-string promise is to hang without any message, is also the current behavior of this pull-request. I am currently throwing an error from a chained then in _connect, but don't know if the best route to take from there is to disconnect, try the next url from _.getNextUrl (this may still be problematic if we cycle over the same url indefinitely!) or some other action all-together. I am eager for suggestions and input on this topic.

The advantage of this pull-request over #113 is that we have a chance to define a previously-undefined behavior, at the expense of additional effort. I understand if the decision is to merge in #113, but I would recommend a section added to the Readme detailing that hanging on an unexpected type from the resolved Promise is a known behavior.

[bennycode]

@pladaria Have you received notifications about this change?

[EricCrosson]

• [x] object with lowercase O (Nice catch! Thank you)
• [ ] express function with generic types

Your use of the word 'generic' gave me an idea, so I ran an experiment and it appears the code works just as well with parametric polymorphism:

const isFunction = <F extends (...args: any) => any>(value: unknown): value is F =>
    typeof value === 'function';

const isPromise = (value: unknown): value is Promise<unknown> =>
    isObject(value) && isFunction((value as any).then);

This code also passes the linter and avoids ignoring the ban-types tslint rule.

However, I'm not sure it makes more sense in this specific case as the reason the Function type is not recommended is that it is generally more-descriptive to write the function parameters and return value explicitly, e.g. Function could be replaced with () => void or (name: string) => number or... But in this case we have no information on the type of parameters nor on the type of the return value. Further, the generic types cannot be inferred since the compiler knows nothing more than we do, so the return type of isFunction above is still value is (...args: any) => any, which provides nothing more than Function.

However, the guard typeof value === 'function' does inform us we're dealing with a Function, which would be preferable to store in return type over unknown.

[pladaria]

Thanks for your PR, I merged #113 because it was just a simple type definition update.

I prefer not to add runtime checks to protect against bad usages coming from plain js calls, if you call

UrlProvider is already strongly typed as:

export type UrlProvider = string | (() => string) | (() => Promise<string>);

Typechecks inside _getNextUrl are done to fetch the URL string from one of the different options, not to protect against bad types. I know this can be opinionated but I don't want to add runtime checks to validate inputs from untyped js

[EricCrosson]

Loud and clear! Thanks @pladaria