Move named exports to properties of default export

[sholladay]

Closes #205

This PR moves HTTPError and TimeoutError to properties of the default export rather than named exports.

CommonJS users can now just use ky instead of ky.default. Also eliminates a warning logged during the build.

[sholladay]

@sindresorhus I couldn't figure out how to update the TypeScript types correctly. I tried making the TimeoutError constructor a property of ky, similar to stop, but then it complained that ky is not a namespace. Could use some help there.

[sindresorhus]

I think you can do:

/**
The error has a response property with the `Response` object.
*/
declare class HTTPError extends Error {
	constructor(response: Response);
	response: Response;
}

/**
The error thrown when the request times out.
*/
declare class TimeoutError extends Error {
	constructor();
}

// ...

declare const ky: {
	...
	readonly HTTPError: HTTPError;
	readonly TimeoutError: TimeoutError;
};

export default ky;

[sholladay]

I tried that, but it didn't work, unfortunately (see the tsd errors in the CI logs). I got even more errors when I tried to make ky a namespace.

[sindresorhus]

Weird. 02818df is passing for me locally.

[sholladay]

Maybe a caching issue? I don't know if tsd uses a cache, but I've occasionally had problems like that with XO where the cache was somehow invalid or out of date and I had to delete the cache to make it notice the new code. Not sure what causes it or how to reproduce it.

[sindresorhus]

Yeah, it was some kind of caching issue. I'm honestly not sure how to make this work.

// @joaovieira @luv-sic

[joaovieira]

My take:

As the error says when you use a type as x.y it looks for the y export in the x namespace. But ky is not a namespace. It's just an exported variable. Doing so, e.g.:

declare namespace ky {
  export class HTTPError extends Error {}
  export class TimeoutError extends Error {}
};

declare function ky(): Promise;

export = ky;

Would make this a CJS module similar to:

module.exports = {
  default: () => {},
  HTTPError: class {},
  TimeoutError: class {},
};

and would then require you to use esModuleInterop flag to importky as a default export in ESM. TS would then decide whether use the namespace or the variable depending on usage.

That said, at a time everyone's switching to ESM, including Node.js (starting from Node 13.2), I'm not sure this is the right approach. It's turning a completely fine ESM into a CJS-like module, limiting it to always have a single export. Moreover, you have the memory footprint of adding classes to every "instance" of ky. Arguably irrelevant, but still, the previous exports had more isolation and semantic meaning (the errors are not related to the "instances" in any way). Same could be said for the stop symbol.

Would also be interested to see the type definitions TSC would output :)

[sindresorhus]

@joaovieira Adding properties to an default exported function is completely fine in JavaScript (ESM), so I'm hoping there's a way to do it in TypeScript too.

[joaovieira]

Yeah, it's fine, but before wasn't wrong either :)

Here's my investigation:

// index.ts
class HTTPError extends Error {
  constructor() {
    super();
    this.name = 'HTTPError';
  }
}

class Ky {
  constructor() {
  }
}

const createInstance = () => {
  const ky = () => new Ky();
  ky.create = () => createInstance();
  ky.HTTPError = HTTPError;
  return ky;
};

export default createInstance();

Results in:

// index.d.ts
declare class HTTPError extends Error {
  constructor();
}
declare class Ky {
  constructor();
}
declare const _default: {
  (): Ky;
  create(): any;
  HTTPError: typeof HTTPError;
};
export default _default;

However, the test still complains it can't find ky namespace:

import {expectType} from 'tsd';
import ky from '.';

expectType<ky.HTTPError>(new ky.HTTPError());

Both from tsd:

And with tsc:

[joaovieira]

InstanceType<typeof ky.HTTPError> in the tests and typeof HTTPError in the type declaration should do it.

Though I'm still of the opinion named exports are better. TypeScript users will have to use that InstanceType<typeof ky.HTTPError> wherever they were previsouly just using HTTPError in type expressions.

[sholladay]

Using typeof and InstanceType did indeed work. Thanks, @joaovieira.

@sindresorhus the tests are passing now and this is ready to merge.

[sindresorhus]

Awesome. This is much nicer 👍