Change chalk.Level from const enum to enum

[elliottsj]

node_modules/chalk/index.d.ts(403,16): error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

Fixes #382

[sindresorhus]

// @KSXGitHub

[KSXGitHub]

@sindresorhus You can merge this.

[sindresorhus]

Is this not a breaking change?

[KSXGitHub]

Is this not a breaking change?

It's not a breaking change. However, TypeScript wouldn't convert Level.Ansi256 to 2 anymore.

[Qix-]

So it is breaking, but only for those treating Level.Ansi256 as a number or using it in e.g. arithmetic, correct?

[fmal]

would be great if this can get merged, it broke my build after upgrading to latest chalk release

[Qix-]

@fmal Major releases should always be considered breaking. If you choose to update a dependency to a major version, be prepared to make some code changes - not all "breaks" are going to be "fixed" like this one.

[SuperWallaby]

I got problem to use by error TS2748 .
Is there a way for temporary use?

[michaelsbradleyjr]

Is this PR still headed for merge or...?

[michaelsbradleyjr]

Any updates on whether this PR may be merged?

[sindresorhus]

If anyone wants to see this merged, please open an issue on TypeScript requesting const enum to be deprecated (and comment the link here). If it cannot be used in practice, it's totally useless and just wastes the time of projects trying to use it.

[michaelsbradleyjr]

@babel/preset-typescript gets about 3.7 million weekly downloads; typescript gets about 7.9 million.

This is not a rigorous analysis... but, because the problem arises with const enum being unsupported by @babel/preset-typescript (because it only works with code that conforms to isolatedModules), that means about half of the typescript projects out there can't use chalk 3.x.

@babel/preset-typescript also doesn't fully support TS namespaces. The TS maintainers have publicly said that ES6 modules are the future; on the other hand, they've also publicly said they have zero plans to remove namespaces from the language. I don't know for certain, but I expect they would not be open to removing const enum either.

There is a plugin (babel-plugin-const-enum) that can help with the const enum situation. However, it's not a wonderful solution because those of us who use tsc to typecheck — which is everyone since babel/preset-typescript doesn't do any typechecking — need to disable the isolatedModules option. But it's important to have that option in place in projects using @babel/preset-typescript, especially in larger projects, to ensure that contributors don't commit code that won't work with it.

In summary: many TypeScript projects won't be able to use chalk 3.x if it retains const enum. It's not really any one's fault, it's just how things are given the technical constraints of @babel/preset-typescript and the practical need to typecheck with tsc in a manner that detects code that isn't compatible.

I'm sure none of the above will convince you to change your mind, and there's certainly no resentment on my part. I wasn't sure how many people who encounter this issue understand the core of the problem, so I hope my comment is helpful. Thanks for all your hard work on this wonderful library!

[sindresorhus]

I don't know for certain, but I expect they would not be open to removing const enum either.

They don't need to remove it, just officially discourage it.

[sindresorhus]

@michaelsbradleyjr Thanks for elaborating. I think you should just copy-paste your above comment into a TypeScript issue to show the TypeScript maintainers that const enum is a real pain-point for users. You can fix Chalk here, but there will be lots of other projects using const enum and are you going to spend time on this every time you encounter a project using const enum?

[michaelsbradleyjr]

Done.

[sindresorhus]

@michaelsbradleyjr Thanks :)

[sindresorhus]

I'm ok with removing the const, but do we need to do anything with

chalk/source/index.js

Lines 221 to 231 in 797461e

	// For TypeScript
	chalk.Level = {
	None: 0,
	Basic: 1,
	Ansi256: 2,
	TrueColor: 3,
	0: 'None',
	1: 'Basic',
	2: 'Ansi256',
	3: 'TrueColor'
	};

?

[elliottsj]

@sindresorhus I believe it's ok to leave that code as-is. The TypeScript-generated code looks different, but is functionally the same as that object:

https://www.typescriptlang.org/play/?ssl=22&ssc=1&pln=21&pc=2#code/KYOwrgtgBAMsBuwA2BRc0DeAoAkAegCoDcBBJJKAYwHslqAnAZygBMBLRgQwCMlgWAdLgJ5cAOWohgUALxQADABosuQsRwAhTozaUoARgBsVWg2aMwAB0sMALkJwjcWnXrn7lqoqTEBlAJJQAEwArMY0dExQFtZ2Dk44JCA6ocZyQZ743jgAKvRgwBEMBsYQbORskiaR5lY29PbCorn5wADCpvSyUADMWAC+WEA

"use strict";
var LevelEnum;
(function (LevelEnum) {
    /**
    All colors disabled.
    */
    LevelEnum[LevelEnum["None"] = 0] = "None";
    /**
    Basic 16 colors support.
    */
    LevelEnum[LevelEnum["Basic"] = 1] = "Basic";
    /**
    ANSI 256 colors support.
    */
    LevelEnum[LevelEnum["Ansi256"] = 2] = "Ansi256";
    /**
    Truecolor 16 million colors support.
    */
    LevelEnum[LevelEnum["TrueColor"] = 3] = "TrueColor";
})(LevelEnum || (LevelEnum = {}));

[RyanCavanaugh]

@sindresorhus we're not going to "officially discourage" anyone from using const enum. Its behavior is well-defined, there are documented trade-offs associated with its use, and just like anything else in a programming language, it can be used in scenarios where it's not appropriate or correct. The usage here was wrong: the matching runtime value exists, and const enum implies it does not exist.

A programmer can write an incorrect TypeScript declaration file using any of our syntax; the author is at fault when this happens, not the specific constructs they used to err with.

Why would you block this good PR, which meaningfully improves the experience of users of this library, for the sake of us telling people not to use a construct which has valid use cases? It's befuddling to me.

[sindresorhus]

The usage here was wrong: the matching runtime value exists, and const enum implies it does not exist.

This is only the case because it was added as an attempt to fix this exact issue: 4e65299

A programmer can write an incorrect TypeScript declaration file using any of our syntax; the author is at fault when this happens, not the specific constructs they used to err with.

The docs does not mention that it will not work for half of TS users. A clear disclaimer in the docs about this would be nice.

Why would you block this good PR, which meaningfully improves the experience of users of this library, for the sake of us telling people not to use a construct which has valid use cases? It's befuddling to me.

This PR is only blocked by it being a breaking change and we were not ready to do a major release yet.

[LinusU]

Hmm, now that 4e65299 was merged this might be a moot point, but before that wouldn't it have been more accurate to type Level as 0 | 1 | 2 | 3. I think that enums makes more sense when the source is ether written in TypeScript and are using enums, or if the source is exporting some kind of object which behaves likes an enum.

When accepting enums, I don't think that the literal values are accepted. So making it an enum makes the JavaScript code read e.g. if (chalk.level > 0) when the TypeScript code reads if (chalk.level > Level.None). To me it makes more sense either if it looked the same in both cases. That is, either it's undocumented that level is a number and it's documented as an object with the levels as properties, so that you do Level.None in normal JavaScript as well, or you type Level as 0 | 1 | 2 | 3 so that the TypeScript codes also reads chalk.level > 0.

Maybe I'm missing some good arguments though 🤔

If we are going to do a breaking change we also have to option to switch to a type union and revert 4e65299, if we think that that would be better...

[sindresorhus]

@LinusU There was a good intention behind this. Level.None is much more readable than 0. But I now agree with you that it's more important to keep the TS and JS the same.

[phawxby]

Can we get something merged soon to get the typings fixed?