TypeScript fixes #217

calebboyd · 2017-10-18T13:30:05Z

Qix- · 2017-10-18T22:29:34Z

@calebboyd Does this PR address #215 (comment)?

calebboyd · 2017-10-18T22:46:12Z

Edit: Yes.

It would solve the need to find and replace the import method. (it adds named exports)

But as for the example:

function logBold(msg): string {
  return chalk.bold(msg)
}

logBold accepts an implicit any, which isn't supported by any signature that we have defined...
If we wanted to support that we could add the type:
(...text: any[]): string here:

chalk/types/index.d.ts

Line 19 in d86db88

(...text: string[]): string;

AFAIK the only supported behavior is accepting string rest args.

Qix- · 2017-10-18T22:47:17Z

@calebboyd is there a way to specify string | ...string[]?

calebboyd · 2017-10-18T22:49:24Z

Since its rest args it has to be an array type.

eg. the following will work.

chalk.bold('a')
chalk.bold('a','b','c')

Is there something missing that isn't captured by that?

calebboyd · 2017-10-19T04:18:29Z

@Qix- I was able to modify the overload order to provide better implicit results.

Also fixed:

an options object is required to return a chalk instance instead of a string
supportsColor is now correctly only defined as a property of the default export and is no longer circular
constructor has its own type to support no options being passed

Sorry for all the churn here... There is more to the API than is immediately apparent....

sindresorhus · 2017-10-19T14:28:04Z

I've changed my mind (Sorry @calebboyd ...). I don't think we should do the named exports. I thought it could be a nice transition step, but it's a day later now and it doesn't seem to be a big problem, and the few that had a problem have fixed it and moved on, so I think it's better for everyone that we just stick to a default export than inconvenience everyone with two ways of using Chalk, one of which is IMHO a bad practice, as I've elaborated on in #215 (comment).

calebboyd · 2017-10-19T14:29:35Z

Okay Cool. (I totally agree) I'll Clean it up. The other fixes are still valid I think

sindresorhus · 2017-10-21T12:02:50Z

types/index.d.ts

 	(...text: string[]): string;
 	(text: TemplateStringsArray, ...placeholders: string[]): string;
-	constructor: Chalk;
+	new (options?: ChalkOptions): Chalk;
+	(options: ChalkOptions): Chalk;


Why are these added here? They're only valid for chalk.constructor()

I missed moving them when I pulled ChalkConstructor out 😬

sindresorhus · 2017-10-21T12:03:37Z

types/index.d.ts

@@ -86,5 +92,6 @@ export interface Chalk {
 	bgWhiteBright: Chalk;
 }

-declare function chalk (): any;
-export default chalk as Chalk;
+declare const chalk: Chalk & { supportsColor: ColorSupport };


Why was & { supportsColor: ColorSupport } moved out of the Chalk interface? Or rather, what's the benefit of having it here?

AFAICT supportsColor only exists on the default export, correct? If it is left in the Chalk interface it will show up all over where it doesn't exist.

You mean that this would be valid as well?

chalk.red.supportsColor

I think you're right, it shouldn't.

That's a good point and I agree, but shouldn't that apply to constructor, enabled, and level too then?

I agree, but that's not what it looks like at runtime.

These things are possible:
chalk.blue.enabled
chalk.blue.constructor
chalk.blue.level

So if it shouldn't be possible, we will have to rewrite some internals so that the type definitions match what we have at runtime.

@SamVerschueren

You mean that this would be valid as well?

Yeah

or it could be changed to behave like enabled and level

Hmm, there might be some use-case where people want to do:

export const color = chalk.blue.bold;

import {color} from '.'; color.enabled = false;

@sindresorhus, good use-case -- and that is supported with these typings. But color.supportsColor would not be (nor does it exist at runtime).

should supportsColor be added to the chained/built chalk properties?

Only if we add it in the runtime, which is a separate discussion to this PR. I'm undecided whether it makes sense though.

alan-agius4 · 2017-10-21T14:10:11Z

types/index.d.ts

@@ -13,20 +13,26 @@ export interface ChalkOptions {
 	level?: Level;
 }

-export interface Chalk {
+export interface ChalkConstructor extends Chalk {


This doesn't look right, Is chalk a class or an object callable with new?

SamVerschueren · 2017-10-22T04:48:12Z

types/index.d.ts

-		hasBasic: boolean;
-		has256: boolean;
-		has16m: boolean;
-	};
 	rgb(r: number, g: number, b: number): Chalk;
 	hsl(h: number, s: number, l: number): Chalk;


Instead of defining that all these methods return Chalk, you can also say that they return this.

rgb(r: number, g: number, b: number): this;

That's also what TS generates for the type definitions. Just another approach though.

imhoffd · 2017-10-23T20:08:19Z

Chiming in here:

Each "color" is an object with a getter:

chalk/index.js

Lines 55 to 64 in f653b06

    
           for (const key of Object.keys(ansiStyles)) { 
        
           	ansiStyles[key].closeRe = new RegExp(escapeStringRegexp(ansiStyles[key].close), 'g'); 
        
           	styles[key] = { 
        
           		get() { 
        
           			const codes = ansiStyles[key]; 
        
           			return build.call(this, this._styles ? this._styles.concat(codes) : [codes], key); 
        
           		} 
        
           	}; 
        
           }

Setting these values results in a run-time error: TypeError: Cannot set property green of #<Chalk> which has only a getter.

So these typings should change from:

	reset: this;
	bold: this;
	dim: this;
...

to:

	readonly reset: this;
	readonly bold: this;
	readonly dim: this;
...

calebboyd · 2017-10-24T02:25:18Z

@dwieeb You're right!

sindresorhus · 2017-10-24T04:02:49Z

Thank you so much for helping out with this @calebboyd :)

fix(types): remove arbitrary value expression

381f4ab

calebboyd changed the title ~~Named exports~~ TypeScript named exports Oct 19, 2017

This was referenced Oct 19, 2017

Remove arbitrary value expression in the TS type definition #216

Closed

Add TypeScript definitions #207

Merged

fix(types): change overload order for better implicit any inference

f6b0e45

calebboyd changed the title ~~TypeScript named exports~~ Typescript fixes Oct 19, 2017

pelotom mentioned this pull request Oct 21, 2017

Fix export typing #220

Closed

sindresorhus reviewed Oct 21, 2017

View reviewed changes

sindresorhus changed the title ~~Typescript fixes~~ TypeScript fixes Oct 21, 2017

sindresorhus requested a review from SamVerschueren October 21, 2017 12:04

alan-agius4 reviewed Oct 21, 2017

View reviewed changes

fix: chalk.constructor typedef

37a4d59

SamVerschueren reviewed Oct 22, 2017

View reviewed changes

refactor(types): chain this instead of Chalk in Chalk interface

d9b67a9

pelotom mentioned this pull request Oct 22, 2017

fix(typings): fix error, the expression of an export assignment must… #221

Closed

fix: make style properties readonly

e43ec91

sindresorhus approved these changes Oct 24, 2017

View reviewed changes

Merge branch 'master' into named-exports

56f1e26

sindresorhus merged commit 7be154c into chalk:master Oct 24, 2017

calebboyd deleted the named-exports branch October 24, 2017 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript fixes #217

TypeScript fixes #217

calebboyd commented Oct 18, 2017 •

edited

Qix- commented Oct 18, 2017

calebboyd commented Oct 18, 2017 •

edited

Qix- commented Oct 18, 2017 •

edited

calebboyd commented Oct 18, 2017 •

edited

calebboyd commented Oct 19, 2017 •

edited

sindresorhus commented Oct 19, 2017

calebboyd commented Oct 19, 2017

sindresorhus Oct 21, 2017

calebboyd Oct 22, 2017

sindresorhus Oct 21, 2017

calebboyd Oct 22, 2017 •

edited

SamVerschueren Oct 22, 2017

sindresorhus Oct 22, 2017

SamVerschueren Oct 22, 2017

calebboyd Oct 22, 2017 •

edited

sindresorhus Oct 23, 2017

calebboyd Oct 23, 2017

sindresorhus Oct 24, 2017

alan-agius4 Oct 21, 2017

SamVerschueren Oct 22, 2017

calebboyd Oct 22, 2017

imhoffd commented Oct 23, 2017

calebboyd commented Oct 24, 2017

sindresorhus commented Oct 24, 2017

TypeScript fixes #217

TypeScript fixes #217

Conversation

calebboyd commented Oct 18, 2017 • edited

Qix- commented Oct 18, 2017

calebboyd commented Oct 18, 2017 • edited

Qix- commented Oct 18, 2017 • edited

calebboyd commented Oct 18, 2017 • edited

calebboyd commented Oct 19, 2017 • edited

sindresorhus commented Oct 19, 2017

calebboyd commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebboyd Oct 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebboyd Oct 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imhoffd commented Oct 23, 2017

calebboyd commented Oct 24, 2017

sindresorhus commented Oct 24, 2017

calebboyd commented Oct 18, 2017 •

edited

calebboyd commented Oct 18, 2017 •

edited

Qix- commented Oct 18, 2017 •

edited

calebboyd commented Oct 18, 2017 •

edited

calebboyd commented Oct 19, 2017 •

edited

calebboyd Oct 22, 2017 •

edited

calebboyd Oct 22, 2017 •

edited