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

Minor version 2.2 is breaking for TypeScript consumers #215

Closed
felixfbecker opened this issue Oct 18, 2017 · 41 comments
Closed

Minor version 2.2 is breaking for TypeScript consumers #215

felixfbecker opened this issue Oct 18, 2017 · 41 comments

Comments

@felixfbecker
Copy link

felixfbecker commented Oct 18, 2017

sourcegraph/javascript-typescript-langserver#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):

export default chalk as Chalk;

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

chalk/index.js

Line 219 in d86db88

module.exports = Chalk(); // eslint-disable-line new-cap

@sindresorhus
Copy link
Member

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
Copy link
Member

sindresorhus commented Oct 18, 2017

Obligatory XKCD:

workflow

@felixfbecker
Copy link
Author

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

@sindresorhus
Copy link
Member

I've opened a TypeScript issue: microsoft/TypeScript#19283

@timsuchanek
Copy link

timsuchanek commented Oct 18, 2017

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
Copy link
Contributor

@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
Copy link

jkuri commented Oct 19, 2017

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
Copy link

johnnyreilly commented Oct 19, 2017

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 👼 😉

@alan-agius4
Copy link

Created a PR to remove typings from Definitely Type
DefinitelyTyped/DefinitelyTyped#20789

@develar
Copy link

develar commented Oct 23, 2017

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
Copy link

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
Copy link

develar commented Oct 23, 2017

@johnnyreilly
Copy link

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
Copy link

emilio-martinez commented Oct 24, 2017

@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
Copy link
Member

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

@sindresorhus
Copy link
Member

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
Copy link
Member

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

@felixfbecker
Copy link
Author

felixfbecker commented Oct 24, 2017

@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

"test": "xo && tsc --project types && nyc ava",

@sindresorhus
Copy link
Member

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

@sindresorhus
Copy link
Member

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
Copy link
Author

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-
Copy link
Member

Qix- commented Oct 24, 2017

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
Copy link

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
Copy link
Member

sindresorhus commented Oct 24, 2017

@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
Copy link
Member

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
Copy link

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
Copy link

develar commented Oct 24, 2017

@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
Copy link
Contributor

@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
Copy link

@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
Copy link
Author

@calebboyd even with preserveConstEnums?

@calebboyd
Copy link
Contributor

@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
Copy link

celsoe commented Oct 26, 2017

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?

dominique-mueller added a commit to dominique-mueller/automatic-release that referenced this issue Oct 30, 2017
- Update dependencies and devDependencies to latest versions
- Fix chalk import (see chalk/chalk#215)
- Fix git merge command
@Qix-
Copy link
Member

Qix- commented Nov 6, 2017

@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
Copy link

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/**/*"
  ]
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests