Navigation Menu

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

Deprecate chalk.constructor() in favor of new chalk.Instance() #322

Merged
merged 10 commits into from Mar 12, 2019

Conversation

tom-sherman
Copy link
Contributor

@tom-sherman tom-sherman commented Jan 26, 2019

Closes #188

Todo:

  • Somehow deprecate calls to chalk.constructor()
  • Update readme

Here is the output from npm run bench:

(node:12838) [DEP0006] DeprecationWarning: child_process: options.customFds option is deprecated. Use options.stdio instead.

                      chalk
         191,041 op/s » single style
          64,176 op/s » several styles
         391,893 op/s » cached styles
          68,051 op/s » nested styles


  Suites:  1
  Benches: 4
  Elapsed: 4,760.06 ms

Compared to current version of chalk:

                      chalk
         193,479 op/s » single style
          67,814 op/s » several styles
         408,394 op/s » cached styles
          67,900 op/s » nested styles


  Suites:  1
  Benches: 4
  Elapsed: 4,706.14 ms

@tom-sherman
Copy link
Contributor Author

const ctx1 = new chalk.instance()
const ctx2 = new chalk.instance({enabled: false})

console.log(
  ctx1`{bold.rgb(10,100,200) Hello!}`,
  ctx2`{bold.rgb(10,100,200) Hello2!}`,
  chalk.blue('blue')
)

The above code will output:

screenshot 2019-01-26 at 14 35 20

@sindresorhus
Copy link
Member

Can you add some tests?

@sindresorhus
Copy link
Member

And update all the references to .constructor()?

❯ rgs constructor
index.js.flow
27:	constructor(options?: Options): Chalk,

index.js
29:	// We check for this.template here since calling `chalk.constructor()`
40:		chalk.template.constructor = Chalk;

index.test-d.ts
19:// -- Constructor --
20:expectType<Chalk>(new chalk.constructor({level: 1}));

readme.md
143:Chalk is enabled by default unless explicitly disabled via the constructor or `chalk.level` is `0`.
148:const ctx = new chalk.constructor({enabled: false});
158:const ctx = new chalk.constructor({level: 0});

test/chalk.js
86:	t.is(new chalk.constructor({level: 1}).hex('#FF0000')('hello'), '\u001B[91mhello\u001B[39m');
87:	t.is(new chalk.constructor({level: 1}).bgHex('#FF0000')('hello'), '\u001B[101mhello\u001B[49m');
91:	t.is(new chalk.constructor({level: 2}).hex('#FF0000')('hello'), '\u001B[38;5;196mhello\u001B[39m');
92:	t.is(new chalk.constructor({level: 2}).bgHex('#FF0000')('hello'), '\u001B[48;5;196mhello\u001B[49m');
93:	t.is(new chalk.constructor({level: 3}).bgHex('#FF0000')('hello'), '\u001B[48;2;255;0;0mhello\u001B[49m');
97:	t.is(new chalk.constructor({level: 0}).hex('#FF0000')('hello'), 'hello');
98:	t.is(new chalk.constructor({level: 0}).bgHex('#FF0000')('hello'), 'hello');

index.d.ts
38:export interface Constructor {
78:	constructor: Constructor;

test/visible.js
9:	const instance = new chalk.constructor({level: 3, enabled: true});
15:	const instance = new chalk.constructor({level: 3, enabled: false});
21:	const instance = new chalk.constructor({level: 0, enabled: true});
27:	const instance = new chalk.constructor({level: 3, enabled: true});

test/constructor.js
9:	const instance = new chalk.constructor({level: 0, enabled: true});
17:	const instance = new chalk.constructor({enabled: false});
27:		new chalk.constructor({level: 10});
31:		new chalk.constructor({level: -1});

test/_flow.js
5:chalk.constructor({levl: 1});
6:chalk.constructor({level: 1});
9:new chalk.constructor({enabled: 'true'});
10:new chalk.constructor({enabled: true});
25:const badCtx = chalk.constructor({level: 4});
26:const ctx = chalk.constructor({level: 3});
44:const chalkInstance = new chalk.constructor();

test/template-literal.js
10:	const instance = chalk.constructor();
15:	const instance = chalk.constructor({level: 0});
20:	const instance = chalk.constructor({level: 0});
26:	const instance = chalk.constructor({level: 0});
34:	const instance = chalk.constructor({level: 3});
47:	const instance = chalk.constructor({level: 3});
53:	const instance = chalk.constructor({level: 3});
70:	const instance = chalk.constructor({level: 3});
80:	const instance = chalk.constructor({level: 3});
100:	const instance = chalk.constructor({level: 0});
106:	const instance = chalk.constructor({level: 3});
112:	const instance = chalk.constructor({level: 0});
118:	const instance = chalk.constructor({level: 0});
123:	const instance = chalk.constructor({level: 0});
129:	const instance = chalk.constructor({level: 0});
135:	const instance = chalk.constructor({level: 3});
142:	const instance = chalk.constructor({level: 3}); // Keep level at least 1 in case we optimize for disabled chalk instances
152:	const instance = chalk.constructor({level: 0});
162:	const instance = chalk.constructor({level: 0});
167:	const instance = chalk.constructor({level: 0});

@sindresorhus
Copy link
Member

You also need to update the TypeScript and Flow type definition.

@sindresorhus
Copy link
Member

Somehow deprecate calls to chalk.constructor()

Add a dummy function that throws a helpful error.

@sindresorhus sindresorhus changed the title Add instance() method to support new Chalk.instance() Add instance() method to support new Chalk.instance() Jan 26, 2019
@sindresorhus
Copy link
Member

Should we make it chalk.Instance()? I ask because a lot of people have linters that will complain if you try to new something that is not PascalCase.

@tom-sherman
Copy link
Contributor Author

tom-sherman commented Jan 26, 2019

Thanks, I'll make these changes.

Tests are failing due to that exact problem, the other solution I can think of is to expose a helper function of chalk.createInstance(...) or similar?

nyc ava && tsd-check && flow passes, it's just the linter which is failing right now.

@tom-sherman tom-sherman changed the title Add instance() method to support new Chalk.instance() Add Instance() method to support new Chalk.Instance() Jan 29, 2019
@tom-sherman
Copy link
Contributor Author

@sindresorhus I've changed all references to use a capital letter (new chalk.Instance()). Is this ready to be merged?

index.js Outdated
return chalk.template;
}
chalk.template.constructor = () => {
throw new Error('Chalk.constructor() is deprecated. Use new Chalk.Instance() instead.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new Error('Chalk.constructor() is deprecated. Use new Chalk.Instance() instead.');
throw new Error('`chalk.constructor()` is deprecated. Use `new chalk.Instance()` instead.');

readme.md Outdated
@@ -140,12 +140,12 @@ Multiple arguments will be separated by space.

Color support is automatically detected, as is the level (see `chalk.level`). However, if you'd like to simply enable/disable Chalk, you can do so via the `.enabled` property. When `chalk.enabled` is `true`, `chalk.level` must *also* be greater than `0` for colored output to be produced.

Chalk is enabled by default unless explicitly disabled via the constructor or `chalk.level` is `0`.
Chalk is enabled by default unless explicitly disabled via `Chalk.Instance()` or `chalk.level` is `0`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Chalk is enabled by default unless explicitly disabled via `Chalk.Instance()` or `chalk.level` is `0`.
Chalk is enabled by default unless explicitly disabled via `new chalk.Instance()` or `chalk.level` is `0`.

@sindresorhus
Copy link
Member

Sorry, this got totally lost in my open tabs.

@tom-sherman
Copy link
Contributor Author

@sindresorhus No problem! I've made the suggested changes.

@sindresorhus sindresorhus changed the title Add Instance() method to support new Chalk.Instance() Deprecate chalk.constructor() in favor of new chalk.Instance() Mar 12, 2019
@sindresorhus sindresorhus merged commit de2f4cd into chalk:master Mar 12, 2019
@sindresorhus
Copy link
Member

Thank you! :)

@cowwoc
Copy link

cowwoc commented Mar 28, 2019

As a new chalk user, I've got to ask: why?

What was wrong with new chalk()?

@Qix-
Copy link
Member

Qix- commented Mar 28, 2019

new chalk() never should have worked.

@cowwoc
Copy link

cowwoc commented Mar 28, 2019

@Qix- Okay, but this doesn't explain what was actually wrong with it. How does moving to new chalk().Instance() help?

@Qix-
Copy link
Member

Qix- commented Mar 29, 2019

Because chalk was already an instance of chalk. Using new on it doesn't make much semantic sense.

But moreover, IIRC there was a limitation with using new chalk, something with the template parser.

Copy link

@DeveloperNeon DeveloperNeon left a comment

Choose a reason for hiding this comment

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

Screws Stuff Up For Me

@tom-sherman
Copy link
Contributor Author

tom-sherman commented Apr 17, 2020

@grunge6 Please raise an issue with replication steps if you have found a bug or would like some support 😄

@Qix-
Copy link
Member

Qix- commented Apr 18, 2020

@grunge6 Such is OSS. We can't retain backwards compatibility forever, especially when we are careful about following semantic versioning.

If there is a bug, please open a new issue so we can address it. Be sure to include all of the relevant information (such as how to reproduce the bug).

Locking this as there is nothing further actionable.

@chalk chalk locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to class-based construction
5 participants