chalk / chalk

🖍 Terminal string styling done right
MIT License
21.82k stars 857 forks source link

Minor version 2.2 is breaking for TypeScript consumers #215

Closed felixfbecker closed 6 years ago

felixfbecker commented 6 years ago

https://github.com/sourcegraph/javascript-typescript-langserver/issues/371 https://travis-ci.org/sourcegraph/javascript-typescript-langserver/jobs/289329597#L1487-L1490

src/logging.ts(72,40): error TS2339: Property 'grey' does not exist on type 'typeof "/home/travis/build/sourcegraph/javascript-typescript-langserver/node_modules/chalk/types/...'.
src/logging.ts(80,40): error TS2339: Property 'bgCyan' does not exist on type 'typeof "/home/travis/build/sourcegraph/javascript-typescript-langserver/node_modules/chalk/types/...'.
src/logging.ts(88,40): error TS2339: Property 'bgYellow' does not exist on type 'typeof "/home/travis/build/sourcegraph/javascript-typescript-langserver/node_modules/chalk/types/...'.
src/logging.ts(96,40): error TS2339: Property 'bgRed' does not exist on type 'typeof "/home/travis/build/sourcegraph/javascript-typescript-langserver/node_modules/chalk/types/...'.

Before 2.2, with @types/chalk, it was possible to import chalk like this:

https://github.com/sourcegraph/javascript-typescript-langserver/blob/6fcc4e1df9283dbd84db9411aa26e9f49556cb50/src/logging.ts#L2

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/05ac46f9a9307d43e082b7cec4b32f40df3a3b58/types/chalk/index.d.ts#L6

2.2 brings its own types, which override @types/chalk, but they don't allow this import (only a defaut import):

https://github.com/chalk/chalk/blob/d86db88e778fa856f4d6f5f68c588750ca06b822/types/index.d.ts#L90

Although it seems to work at runtime and appears semantically correct / correct in regard to the JS:

https://github.com/chalk/chalk/blob/d86db88e778fa856f4d6f5f68c588750ca06b822/index.js#L219

sindresorhus commented 6 years ago

Sorry about the trouble, but this seems more like a problem with the DefinitelyTyped way of handling typings. Projects can't really take into account unofficial type definitions. We went with a default export now as that's more future proof. It's not a breaking change, as Chalk didn't officially come with typings before.

Maybe you should open an issue on TypeScript and request that @types/chalk kind of typings takes priority over ones that come with packages?

sindresorhus commented 6 years ago

Obligatory XKCD:

workflow
felixfbecker commented 6 years ago

Shouldn't the TypeScript typings reflect what's available at runtime / to JS consumers? I.e. if you remove the module.exports for JS users at some point, that would be a major version and then it can be removed for TS consumers in the same update

sindresorhus commented 6 years ago

@felixfbecker It's going to be years until we can use native modules and default export in vanilla JS. I don't see why TS users should have to wait.

felixfbecker commented 6 years ago

I don't think so unfortunately - but what is better about

import chalk from 'chalk'

vs

import * as chalk from 'chalk'
import { red, green } from 'chalk'

? The latter seems more flexible to me. Or are there any changes planned to the Chalk API that would disallow this kind of import, e.g. in a native ES6 environment because the chalk has to be a Chalk instance?

sindresorhus commented 6 years ago

Chalk is by nature a default export. The fact that you can do import { red, green } from 'chalk' is just because CommonJS default export is not enforced as an ES2015 default export when used with TS or Babel. I also don't think import { red, green } from 'chalk' is a good pattern. You import these highly generic variable names into the module scope and have to update it every time you need a new style. Chalk is the import. The styles (like red, green) are just modifiers, not exports. If you want the colors as variables, you can simply do const {red, green} = chalk;.

You will have to change import * as chalk from 'chalk' at some point regardless, as that's how it will work when we convert Chalk to be an ES2015 module sometime in the future.

felixfbecker commented 6 years ago

Makes sense. Feel free to close or leave this issue open a few days for TS folks who might meet the same issue and come here

sindresorhus commented 6 years ago

Thanks for understanding. I'll keep this issue open for a while.

sindresorhus commented 6 years ago

I've opened a TypeScript issue: https://github.com/Microsoft/TypeScript/issues/19283

timsuchanek commented 6 years ago

Another issue I have with the new TypeScript definitions:

function logBold(msg): string {
  return chalk.bold(msg)
}
 error TS2322: Type 'Chalk' is not assignable to type 'string'
import * as chalk from 'chalk'

being broken is fixed for me with a search+replace to

import chalk from 'chalk'
calebboyd commented 6 years ago

@timsuchanek You're passing an implicit any (msg) to chalk, which only accepts a string. If you add the correct type your error will go away.

jkuri commented 6 years ago

I also believe current definitions are not done okay and should be exported as namespace first. Upgrading chalk to a new version broked all my projects currently, why not simply use ones from the @types/chalk and update this typings if needed?

johnnyreilly commented 6 years ago

I've also just been broken by this. Despite initial thrashing, the changeover is not too painful. Since examples are useful I thought I'd share the changes I made to switch to this:

Previous import examples:

import { constructor as ChalkConstructor, Chalk, ChalkChain } from 'chalk';

Now looks like:

import chalk from 'chalk';

Usage looks like goes from this:

    const colors = new ChalkConstructor({ enabled: loaderOptions.colors });

const makeLogInfo = (loaderOptions: LoaderOptions, logger: InternalLoggerFunc, green: ChalkChain) =>
//...

function successfulTypeScriptInstance(
    // ...
    colors: Chalk,
    // ...
) {
// ...
}

To this:

    const colors = new chalk.constructor({ enabled: loaderOptions.colors });

const makeLogInfo = (loaderOptions: LoaderOptions, logger: InternalLoggerFunc, green: typeof chalk) =>
//...

function successfulTypeScriptInstance(
    // ...
    colors: typeof chalk,
    // ...
) {
// ...
}

PS every time a package on npm starts shipping it's own *.d.ts an angel gets his wings :angel: :wink:

alan-agius4 commented 6 years ago

Created a PR to remove typings from Definitely Type https://github.com/DefinitelyTyped/DefinitelyTyped/pull/20789

develar commented 6 years ago

Sorry, 5 days passed, but chalk 2.2 is still not usable for TypeScript 2.6. Could you please release a new version to make chalk 2.2 compilable again? As workaround I have set version to 2.1, but :( it is not good.

johnnyreilly commented 6 years ago

I'm not sure that's true. ts-loader runs against TypeScript@next and it's okay: https://travis-ci.org/TypeStrong/ts-loader

However, 2.2 did seem to be a breaking changes release

develar commented 6 years ago

@johnnyreilly https://github.com/chalk/chalk/pull/216 (https://github.com/chalk/chalk/pull/207#issuecomment-337462922)

johnnyreilly commented 6 years ago

Thanks for the link - it seems that ts-loader is okay with TypeScript@next so my working assumption is that this is OK. Can you think why that wouldn't be the case? I'm sure I'll learn quickly if it isn't!

emilio-martinez commented 6 years ago

@sindresorhus Totally understand the issue with the unofficial type definitions, and absolutely agree with the XKCD. Regardless, IMO, the type definitions output with chalk@2.2.0 seem unintuitive, but that may be just me. See the two examples below which arguably can be equally as common as import chalk from 'chalk'.

On the first, I would expect that to behave just like const chalk = require('chalk'). Oddly, you get default and Level, of which the latter doesn't seem to exist in the source js. On the second, while I agree that it may be a bad pattern for chalk, it seems unintuitive that a user would get two interfaces (Chalk and ChalkOptions) and a Level enum, but no access to the actual chalk colors.

chalk@2.2.0 start import chalk@2.2.0 destructured import

Like I said, maybe it's just me, but I found it unintuitive since I never import like import chalk from 'chalk'. In any case, agree with @johnnyreilly; this not a huge deal to work around and always appreciated when packages ship with official definitions.

sindresorhus commented 6 years ago

@develar TypeScript 2.5 is the latest official version. 2.6 is still RC from what I can tell.

sindresorhus commented 6 years ago

On the first, I would expect that to behave just like const chalk = require('chalk').

You can't compare it to require. ES2015 import works differently. This has nothing to do with Chalk specifically, though.

Oddly, you get default and Level, of which the latter doesn't seem to exist in the source js.

The Level enum is exported for convenience for TS users. There's no way to represent enums in JS. You don't have to use it though. You can use plain integers instead.

On the second, while I agree that it may be a bad pattern for chalk, it seems unintuitive that a user would get two interfaces (Chalk and ChalkOptions) and a Level enum, but no access to the actual chalk colors.

The Chalk colors are part of the Chalk class and not part of the exported interface directly for reasons explained in my earlier comment.

would get two interfaces (Chalk and ChalkOptions)

How would you expose type-strong options?


We're always happy to consider improvements, but we're not interested in changing how the default export works. That's set in stone.

sindresorhus commented 6 years ago

A new release is out with some TypeScript improvements: https://github.com/chalk/chalk/releases/tag/v2.3.0

felixfbecker commented 6 years ago

The Level enum is exported for convenience for TS users. There's no way to represent enums in JS. You don't have to use it though. You can use plain integers instead.

Enums compile to an object with a property for every enum member in JS, therefor it creates both a type and a value in the type system.

If this enum is not present at runtime, the right way to express it is with just a type:

export type Level = 0 | 1 | 2 | 3

Otherwise this will lead to bugs when TS consumers think they can use e.g. Level.Basic in their code but it is undefined at runtime.

sindresorhus commented 6 years ago

@felixfbecker Seems to work fine in our test: https://github.com/chalk/chalk/blob/14e0aa97727019b22f0a003fdc631aeec5e2e24c/types/test.ts#L29

felixfbecker commented 6 years ago

@sindresorhus is that test file only compiled or actually executed? Judging from the package.json it looks like the file is only compiled, which is expected to work, but it will be a runtime error

https://github.com/chalk/chalk/blob/14e0aa97727019b22f0a003fdc631aeec5e2e24c/package.json#L11

sindresorhus commented 6 years ago

@felixfbecker That's a good point. We should execute it too.

sindresorhus commented 6 years ago
export type Level = 0 | 1 | 2 | 3

Is there any way to have named constant here or at least some kind of documentation for those magic numbers?

felixfbecker commented 6 years ago

Sure, you can explain it in a docblock of the type alias. Will be more visible though if explained on the actual parameter/property that uses it.

Or, provide it at runtime for JS consumers too ;)

Qix- commented 6 years ago

Maybe this is a stupid question but why don't we expose typescript bindings in the respective module, supports-color, and then expose those defs in chalk? Is that possible? (I don't know enough about TS).

donaldpipowitch commented 6 years ago

The Chalk colors are part of the Chalk class and not part of the exported interface directly for reasons explained in my earlier comment.

Didn't know that. I loved my import { red } from 'chalk';. It would be so nice, if this could officially be added in the future again.

Somehow the "downsides" never really applied to me:

You import these highly generic variable names into the module scope and have to update it every time you need a new style.

The generic names made my easy to read, I never really had troubles with name conflicts and I never needed more than three or four colours at once. (Also auto-importing and removing unused imports made this very ergonomic.)

sindresorhus commented 6 years ago

@Qix- Too much overhead and boilerplate. I don't want TS boilerplate in all of our tiny modules. Almost nobody uses supportsColor directly. Easier to just do it here.

sindresorhus commented 6 years ago

Didn't know that. I loved my import { red } from 'chalk';. It would be so nice, if this could officially be added in the future again.

That won't happen. As I've commented earlier, you can just do:

import chalk from 'chalk';
const {red} = chalk;
donaldpipowitch commented 6 years ago

That won't happen. As I've commented earlier, you can just do:

Yeah, I read you comment and I know I can do that. Just wanted to give some feedback, because I personally prefer the other way and haven't encountered downsides.

develar commented 6 years ago

@donaldpipowitch Position is that Chalk it is class and exported chalk it is instance of Chalk class. And red and other are methods. So, it is truly correct and nothing to discuss here (yes, I personally also use and like import { red } from 'chalk').

calebboyd commented 6 years ago

@sindresorhus

Is there any way to have named constant here or at least some kind of documentation for those magic numbers?

image

The const enum is documenting it -- It also compiles to the actual values so the above snippet becomes

var chalk_1 = require("chalk");
chalk_1.default.level = 2 /* Ansi256 */;

@felixfbecker

therefor it creates both a type and a value in the type system.

const enum s are are fully replaced at compile time. (see above snippet -- very different from a regular enum)

mceachen commented 6 years ago

@sindresorhus thanks for Chalk and your other contributions!

You may not appreciate it, and I know you didn't author it, but @types/chalk for all intents and purposes was the "official" Chalk API for TypeScript users. Creating an index.d.ts that differs from that API is a breaking change, by definition. The other commenters on this issue are evidence to this effect.

Also, I've used upwards of 100 typed packages over the past year, and this typing is the first to require a non-namespaced import chalk from "chalk" style. I'd expect a good deal of issues or questions to arise from this.

For other people wondering why chalk broke for them with a minor release, as stated above, change

import * as chalk from "chalk"

to

import chalk from "chalk"
felixfbecker commented 6 years ago

@calebboyd even with preserveConstEnums?

calebboyd commented 6 years ago

@felixfbecker Yep! That only effects what typescript emits (your code). Not declarations within types provided by modules you're using (eg chalk).

@mceachen The updated type definitions actually fix a few bugs.

import * as chalk from 'chalk'
console.log(typeof chalk) //function

The above syntax for extracting an object of type function from a module should not be possible. While many transpilers allow this, it is not valid es2015

The previous types also did not capture chalk (the default export) as a function, which it is.

celsoe commented 6 years ago

Hello, I'm having a problem that is probably related to this issue. I have no idea why but when I try to use chalk from the typescript generated code it outputs no color and upon checking the chalk object, it says that no colors are supported (supportsColor: false)

I installed both chalk and @types/chalk (latest versions, although 2.2.0 has the same issue) I tried doing the same thing with plain js by creating a foo.js file and running it with the same chalk module and it works normally. Only with the typescript generated bundle this issue happens, as you can see in the pictures:

This is the foo.js output: screenshot from 2017-10-26 14-22-17 And the code:

const chalk = require('chalk');

console.log(chalk.red('Red'));
console.log(chalk);

This is the output of the TypeScript generated bundle: screenshot from 2017-10-26 14-22-12

And the code:

import * as chalk from "chalk";

export function simulate() {
    console.log(chalk.red('Red'));
    console.log(chalk);
}

Any clues?

Qix- commented 6 years ago

@celsoe that's not related to TypeScript - that's because you're running it in a wrapper of some sort that is causing isatty() to return 0. Either pass --colors or set FORCE_COLOR=1 in your environment.

ZenoTian commented 6 years ago

modify tsconfig.json, add "node_modules/types" to typeRoots

{
  "compilerOptions": {
      "module": "commonjs",
      "target": "es6",
      "noImplicitAny": false,
      "moduleResolution": "node",
      "sourceMap": true,
      "outDir": "dist",
      "baseUrl": ".",
      "typeRoots": [
        "node_modules/@types",
        "node_modules/types" //it's for chalk
      ]
  },
  "include": [
      "src/**/*"
  ]
}