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

Add TypeScript definitions #207

Merged
merged 15 commits into from
Oct 18, 2017
Merged

Add TypeScript definitions #207

merged 15 commits into from
Oct 18, 2017

Conversation

calebboyd
Copy link
Contributor

@calebboyd calebboyd commented Sep 9, 2017

This is just a continuation of #169 based on the conversation there. Re-based and ready to go if it LGTY

/cc @t-sauer


Closes #169

types/index.d.ts Outdated
@@ -1,87 +1,89 @@
// Type definitions for Chalk
// Definitions by: Thomas Sauer <https://github.com/t-sauer>
export const enum Level {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using const here will cause replacements to occur during compilation -- Since this value doesn't exist at runtime

@calebboyd
Copy link
Contributor Author

@t-sauer Does this LGTY?

@sindresorhus sindresorhus changed the title TypeScript Definitions TypeScript definitions Oct 15, 2017
@sindresorhus sindresorhus changed the title TypeScript definitions Add TypeScript definitions Oct 15, 2017
types/index.d.ts Outdated
has256: boolean,
has16m: boolean
}
rgb: (r: number, g: number, b: number) => Chalk

Choose a reason for hiding this comment

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

You could write them like rgb(r: number, g: number, b: number): Chalk. I believe this is the preferred way.

types/index.d.ts Outdated
}

export interface Chalk {
new (options: ChalkOptions): Chalk

Choose a reason for hiding this comment

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

Add a ; after every line.

types/index.d.ts Outdated
bgWhiteBright: Chalk
}

declare function chalk (): any

Choose a reason for hiding this comment

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

;

types/index.d.ts Outdated
}

declare function chalk (): any
export default chalk as Chalk

Choose a reason for hiding this comment

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

;

@calebboyd
Copy link
Contributor Author

@SamVerschueren updated

Copy link

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

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

One more change and then I believe it's good to go.

types/index.d.ts Outdated
enabled: boolean;
level: Level;
supportsColor: {
level: Level,

Choose a reason for hiding this comment

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

Last nitpick :), also ;.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ammended :)

types/index.d.ts Outdated
None = 0,
Basic = 1,
Extended = 2,
TrueColor = 3
Copy link
Member

Choose a reason for hiding this comment

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

Can we change Extended to Ansi256 here? That's the more correct term; I've never seen extended used to refer to the Ansi256 space.

types/index.d.ts Outdated
}

export interface Chalk {
new (options: ChalkOptions): Chalk;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be options?: ChalkOptions? I don't know enough about TS defs, but we don't require an object be passed.


chalk.grey('foo');

chalk.constructor({level: 1});
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for chalk.constructor() and new chalk.constructor()?

"target": "es6",
"noImplicitAny": true,
"noEmit": true,
"allowJs": true
Copy link
Member

Choose a reason for hiding this comment

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

De-indent the properties for compilerOptions by 1, please :)

@Qix-
Copy link
Member

Qix- commented Oct 16, 2017

Thank you so much for your continued attention to this. I know we aren't the fastest with getting back to this but the promptness is very much appreciated <3

@calebboyd
Copy link
Contributor Author

calebboyd commented Oct 16, 2017

@Qix- Good finds. Updated.

@Qix-
Copy link
Member

Qix- commented Oct 16, 2017

LGTM 👍 Thank you!

@Qix-
Copy link
Member

Qix- commented Oct 16, 2017

Will defer to @sindresorhus as to how he wants to move forward with a merge/release.

Thank you so much @calebboyd ❤️ 💃

@sindresorhus sindresorhus merged commit f653b06 into chalk:master Oct 18, 2017
@sindresorhus
Copy link
Member

Awesome! Thanks to @t-sauer for starting this and @calebboyd for finishing it. Really appreciate your work on this.

@develar
Copy link

develar commented Oct 18, 2017

Hmm.... have you checked it ;)? TS 2.6.0

~/electron-builder/node_modules/chalk/types/index.d.ts (90, 16): The expression of an export assignment must be an identifier or qualified name in an ambient context.

@calebboyd
Copy link
Contributor Author

@develar #216

@donaldpipowitch
Copy link

Thank you for this PR. I just noticed that I can't do import { red } from 'chalk'; anymore. Only import chalk from 'chalk'; const { red } = chalk;.

Is this intended? :)

@calebboyd
Copy link
Contributor Author

@donaldpipowitch It certainly wasn't the plan to break people's workflow. #217 if merged will add named exports to the type definitions

@donaldpipowitch
Copy link

No problem. Thank you very much. I love it when packages ship their own typings. Thank you for the work.

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 this pull request may close these issues.

None yet

7 participants