Skip to content

Make the TypeScript types strict and better #849

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"node": ">=8.6"
},
"scripts": {
"test": "xo && nyc ava",
"test": "xo && nyc ava && tsc --noEmit --strict",
"release": "np",
"build": "del-cli dist && tsc",
"prepublishOnly": "npm run build"
Expand Down
2 changes: 1 addition & 1 deletion source/request-as-event-emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export default (options: NormalizedOptions, input?: TransformStream) => {

const rawCookies = typedResponse.headers['set-cookie'];
if (options.cookieJar && rawCookies) {
let promises = rawCookies.map((rawCookie: string) => setCookie!(rawCookie, typedResponse.url!));
let promises: Array<Promise<unknown>> = rawCookies.map((rawCookie: string) => setCookie!(rawCookie, typedResponse.url!));
if (options.ignoreInvalidCookies) {
promises = promises.map(p => p.catch(() => {}));
}
Expand Down
17 changes: 10 additions & 7 deletions source/utils/timed-out.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import {ClientRequest, IncomingMessage} from 'http';
import {Delays} from './types';
import unhandler from './unhandle';

// Help TypeScript understand how we are passing arguments through `setImmediate`.
declare function setImmediate<T extends any[]>(callback: (arg0: number, ...args: T) => void, arg0: number, ...args: T): NodeJS.Immediate;

const reentry = Symbol('reentry');
const noop = (): void => {};

Expand Down Expand Up @@ -32,7 +35,7 @@ export default (request: ClientRequest, delays: Delays, options: TimedOutOptions
const cancelers: Array<typeof noop> = [];
const {once, unhandleAll} = unhandler();

const addTimeout = (delay: number, callback: (...args: unknown[]) => void, ...args: unknown[]): (typeof noop) => {
const addTimeout = <T extends any[]>(delay: number, callback: (delay: number, ...args: T) => void, ...args: T): (typeof noop) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the callback actually take the delay as the first argument? it doesn't seem like it

// Event loop order is timers, poll, immediates.
// The timed event may emit during the current tick poll phase, so
// defer calling the handler until the poll phase completes.
Expand Down Expand Up @@ -91,7 +94,7 @@ export default (request: ClientRequest, delays: Delays, options: TimedOutOptions

if (delays.socket !== undefined) {
const socketTimeoutHandler = (): void => {
timeoutHandler(delays.socket, 'socket');
timeoutHandler(delays.socket!, 'socket');
Copy link
Contributor

@samhh samhh Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the if statement act as a type guard, thus removing the need for the non-null assertion? Likewise in a few other places.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my mistake! 👍

};

request.setTimeout(delays.socket, socketTimeoutHandler);
Expand All @@ -110,15 +113,15 @@ export default (request: ClientRequest, delays: Delays, options: TimedOutOptions

/* istanbul ignore next: hard to test */
if (socket.connecting) {
if (delays.lookup !== undefined && !socketPath && !net.isIP(hostname || host)) {
if (delays.lookup !== undefined && !socketPath && !net.isIP(hostname || host || '')) {
const cancelTimeout = addTimeout(delays.lookup, timeoutHandler, 'lookup');
once(socket, 'lookup', cancelTimeout);
}

if (delays.connect !== undefined) {
const timeConnect = (): (() => void) => addTimeout(delays.connect, timeoutHandler, 'connect');
const timeConnect = (): (() => void) => addTimeout(delays.connect!, timeoutHandler, 'connect');

if (socketPath || net.isIP(hostname || host)) {
if (socketPath || net.isIP(hostname || host || '')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this net.isIP check is used at least twice, can't we abstract it to one central variable?

once(socket, 'connect', timeConnect());
} else {
once(socket, 'lookup', (error: Error): void => {
Expand All @@ -131,14 +134,14 @@ export default (request: ClientRequest, delays: Delays, options: TimedOutOptions

if (delays.secureConnect !== undefined && options.protocol === 'https:') {
once(socket, 'connect', (): void => {
const cancelTimeout = addTimeout(delays.secureConnect, timeoutHandler, 'secureConnect');
const cancelTimeout = addTimeout(delays.secureConnect!, timeoutHandler, 'secureConnect');
once(socket, 'secureConnect', cancelTimeout);
});
}
}

if (delays.send !== undefined) {
const timeRequest = (): (() => void) => addTimeout(delays.send, timeoutHandler, 'send');
const timeRequest = (): (() => void) => addTimeout(delays.send!, timeoutHandler, 'send');
/* istanbul ignore next: hard to test */
if (socket.connecting) {
once(socket, 'connect', (): void => {
Expand Down
7 changes: 5 additions & 2 deletions source/utils/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export interface Response extends http.IncomingMessage {

export type RetryFunction = (retry: number, error: Error | GotError | ParseError | HTTPError | MaxRedirectsError) => number;

export type HandlerFunction = <T extends ProxyStream | CancelableRequest<Response>>(options: Options, next: (options: Options) => T) => T;
export type HandlerFunction = <T extends ProxyStream | CancelableRequest<Response>>(options: NormalizedOptions, next: (options: NormalizedOptions) => T) => T;

export interface RetryOption {
retries?: RetryFunction | number;
Expand Down Expand Up @@ -146,7 +146,7 @@ export interface Options extends Omit<https.RequestOptions, 'agent' | 'timeout'
context?: {[key: string]: unknown};
}

export interface NormalizedOptions extends Omit<Required<Options>, 'timeout' | 'dnsCache' | 'retry'> {
export interface NormalizedOptions extends Omit<Required<Options>, 'timeout' | 'dnsCache' | 'retry' | 'auth' | 'body' | 'port'> {
hooks: Hooks;
gotTimeout: Required<Delays>;
retry: NormalizedRetryOptions;
Expand All @@ -155,6 +155,9 @@ export interface NormalizedOptions extends Omit<Required<Options>, 'timeout' | '
path: string;
hostname: string;
host: string;
auth?: Options['auth'];
body?: Options['body'];
port?: Options['port'];
}

export interface Defaults {
Expand Down
2 changes: 1 addition & 1 deletion source/utils/unhandle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import EventEmitter = require('events');

type Origin = EventEmitter;
type Event = string | symbol;
type Fn = (...args: unknown[]) => void;
type Fn = (...args: any[]) => void;

interface Handler {
origin: Origin;
Expand Down