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

TypeScript definitions for my packages #9

Closed
sindresorhus opened this issue Apr 11, 2019 · 20 comments
Closed

TypeScript definitions for my packages #9

sindresorhus opened this issue Apr 11, 2019 · 20 comments

Comments

@sindresorhus
Copy link
Owner

@BendingBender is helping me adding TypeScript definitions to a lot of my packages: https://github.com/search?q=user%3Asindresorhus+author%3ABendingBender+is%3Apr+typescript 🙌

We can use this space to discuss high-level things.

@sindresorhus
Copy link
Owner Author

@BendingBender Would you be able to prioritize https://github.com/sindresorhus/electron-better-ipc ? I've gotten a couple of emails about doing TypeScript definitions for it.

@BendingBender
Copy link

Will do next.

@BendingBender
Copy link

Note to myself: clarify usage of Array/ReadonlyArray in https://github.com/sindresorhus/typescript-definition-style-guide

@sindresorhus
Copy link
Owner Author

@BendingBender When a function returns an array, should we make it readonly?

foo(bar: string): readonly string[];

Or is it pointless?

@BendingBender
Copy link

I wouldn't do it. I would usually expect a library to give me something I can use for every purpose, not something unnecessary limited. Otherwise I would have to cast/copy the array, this is not very nice ergonomics-wise.

@sindresorhus
Copy link
Owner Author

@BendingBender That's a very good point. So readonly for all input, but never readonly for return values. Correct? If so, I'll update the style guide.

@sindresorhus
Copy link
Owner Author

@BendingBender Would you be able to prioritize https://github.com/sindresorhus/linkify-urls and https://github.com/sindresorhus/linkify-issues? We need it for Refined GitHub.

Context: https://github.com/sindresorhus/refined-github/pull/1783/files#diff-98a7886093e2e560968c34d6c4707b22R11

@sindresorhus
Copy link
Owner Author

@BendingBender Node.js 12 is out, so would be great if you could add it to .travis.yml when you do PRs. Like: sindresorhus/got@bf1aa54#diff-354f30a63fb0907d4ad57269548329e3R3

@BendingBender
Copy link

Will do.

@sindresorhus
Copy link
Owner Author

sindresorhus commented Apr 27, 2019

@BendingBender I know you didn't do this type definition, but would you be able to help convert it to CommonJS-style? https://github.com/chalk/chalk/blob/master/index.d.ts (It can be a breaking change as we're planning a major release there)

@fregante
Copy link

Like sindresorhus/memoize#32 ?

@sindresorhus
Copy link
Owner Author

@bfred-it Yup

@BendingBender
Copy link

I know you didn't do this type definition, but would you be able to help convert it to CommonJS-style? chalk/chalk:index.d.ts@master (It can be a breaking change as we're planning a major release there)

Done.

Repository owner deleted a comment from BendingBender May 9, 2019
@sindresorhus
Copy link
Owner Author

sindresorhus commented May 9, 2019

@BendingBender Is there any way to get VS Code and other editors to show the resolved properties? I have this https://github.com/sindresorhus/crypto-random-string/blob/32365366245e602a58a2a38c4d6fc2edd30355d7/index.d.ts#L52 and it shows up as something completely unreadable:

Screen Shot 2019-05-08 at 17 51 07

Alternatively, a better way to write the definition.

@BendingBender
Copy link

As far as I can see it, there is no better way to type this without giving up the exclusivity of type/characters options.

I think that this definition would be clearer:

interface BaseOptions {...}
interface TypeOptions extends BaseOptions {...}
interface CharactersOptions extends BaseOptions {...}

type Options = MergeExclusive<TypeOptions, CharactersOptions>;

But it is still rendered quite ugly in the suggestion tooltip.

The only other way I can think of is to use discriminated union types, but I have no idea how to do this here without hurting ergonomics:

interface BaseOptions {...}
interface TypeOptions extends BaseOptions {
	kind: 'types';
	type?: 'hex' | 'base64' | 'url-safe';
}
interface CharactersOptions extends BaseOptions {
	kind: 'characters';
	characters?: string;
}

type Options = TypeOptions | CharactersOptions; // <- this one always needs the 'kind' property :-(

@ExE-Boss
Copy link

ExE-Boss commented May 9, 2019

You could do:

interface BaseOptions {  }
interface TypeOptions extends BaseOptions {
	kind: 'types';
	type?: 'hex' | 'base64' | 'url-safe';
}
interface CharactersOptions extends BaseOptions {
	kind: 'characters';
	characters?: string;
}

// No longer needs the `kind` property:
type Options = BaseOptions | TypeOptions | CharactersOptions;

@BendingBender
Copy link

@ExE-Boss Sorry but I've tried this before, this basically renders the type a non-discriminated union type. Thus all properties simply become optional and non-exclusive.

@sindresorhus
Copy link
Owner Author

Hmm, too bad. Really wish TypeScript and VS Code had a better way to handle this.

@sindresorhus
Copy link
Owner Author

@BendingBender Would you be able to prioritize https://github.com/sindresorhus/mimic-response ?

@ExE-Boss
Copy link

I’m in the process of writing the type definitions for mimic-response.

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

No branches or pull requests

4 participants