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

HTTPError type is not exported #237

Closed
hasparus opened this issue Feb 17, 2020 · 3 comments · Fixed by #241
Closed

HTTPError type is not exported #237

hasparus opened this issue Feb 17, 2020 · 3 comments · Fixed by #241

Comments

@hasparus
Copy link
Contributor

hasparus commented Feb 17, 2020

Problem

In 0.13, we could import { HTTPError } from 'ky' in TypeScript.
In 0.17 the value is accessible as ky.HTTPError, but the type is InstanceType<typeof ky.HTTPError>, what's a bit inconvenient.

Possible solution

Instead of declare const ky there could be a namespace ky in index.d.ts. Namespaces can contain types.

@sindresorhus
Copy link
Owner

How is it inconvenient?

@sholladay
Copy link
Collaborator

sholladay commented Feb 17, 2020

I described the rationale for this change in issue #205 and implemented it in PR #205. I highly recommend reading those.

Among other things, this was motivated by the desire to avoid having stop as a named export, which we felt was too vague and prone to naming conflicts with user code, etc. It's better as ky.stop. Unfortunately, I ran into problems trying to make Ky a namespace in TypeScript, which forced me to type it as typeof ky.HTTPError. If you can figure it out, we would likely accept a PR to improve the types. There may be a solution that I'm not aware of, but it seemed like TypeScript just doesn't support default exported functions as namespaces with properties attached to them.

@hasparus
Copy link
Contributor Author

hasparus commented Feb 18, 2020

I also prefer ky.stop instead of named export.

I don't question the rationale behind it.
I am only speaking about the type here.
A class is usually the value A and the type of the instance A in the same namespace.

Following TypeScript code using 0.13

import * as ky from 'ky';

function logStatus(httpError: ky.HTTPError) {
  console.log(httpError.response.status);
}

Looks like this in 0.17

import ky from 'ky';

function logStatus(httpError: InstanceType<typeof ky.HTTPError>) {
  console.log(httpError.response.status);
}

Certainly, "inconvenient" is subjective. I'm sorry for that. Let me rephrase.
I mean that the type of ky.HTTPError instance is accessible, but it is not exported as public API.

I'll give it a shot in a PR tomorrow.


I'll note the general idea here.

Trivial example TypeScript

class HTTPError extends Error {}

const ky = () => {};
ky.HTTPError = HTTPError;

export default ky;

Produces following .d.ts (TS Playground v2 rocks)

declare class HTTPError extends Error {
}
declare const ky: {
    (): void;
    HTTPError: typeof HTTPError;
};
export default ky;

Exactly the current state of affairs :(

However if I add

type _HTTPError = HTTPError;
namespace ky {
  export type HTTPError = _HTTPError;
}

or

namespace ky {
  export type HTTPError = InstanceType<typeof ky.HTTPError>;
}

namespace ky and const ky merge. We have both the ky.HTTPError and ky.HTTPError type.
The lines above are present in .d.ts with additional declare modifier before namespace.

Since ky's .d.ts is maintained separately, my plan is to add this declare namespace ky with HTTPError and TimeoutError types. Something will probably go wrong along the way, but this is the gist of it.

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.

3 participants