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

Move to class-based construction #188

Closed
Qix- opened this issue Jul 31, 2017 · 12 comments · Fixed by #322
Closed

Move to class-based construction #188

Qix- opened this issue Jul 31, 2017 · 12 comments · Fixed by #322
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Milestone

Comments

@Qix-
Copy link
Member

Qix- commented Jul 31, 2017

Issuehunt badges

@sindresorhus right now, we use a traditional function/prototype build up of the Chalk class.

This has the negative side effect that in order to enable templating on custom contexts, you have to omit new - which is counterintuitive.

const ctx1 = chalk.constructor();
const ctx2 = new chalk.constructor();

const str1 = ctx1`hello`; // OK
const str2 = ctx2`hello`; // Error

Unless I'm mistaken, we can leverage constructor()'s ability to return a completely different object (even with new) to allow templates on all new chalk functions.

class Chalk {
    constructor(...) {
        // ... build up chalk template function ...
        return chalkTemplate;
    }

    // ...
}

const ctx = new Chalk();

const str = ctx`hello!`; // OK

Is this something we should do?

// cc @sindresorhus

tom-sherman earned $60.00 by resolving this issue!

@sindresorhus
Copy link
Member

Class syntax requires new though, so that would be a breaking change. We could do it and export a new method instead, maybe new Chalk.instance().

@Qix-
Copy link
Member Author

Qix- commented Aug 7, 2017

True. Should util.deprecate the old function, too.

@wuweiweiwu
Copy link

wuweiweiwu commented Feb 5, 2018

@sindresorhus @Qix- I can take this! implement the new instance function

@sindresorhus
Copy link
Member

@wuweiweiwu Sure, go ahead. Just make sure you run the benchmark so Chalk doesn't regress in performance.

@wuweiweiwu
Copy link

wuweiweiwu commented Feb 6, 2018

@sindresorhus I have a question. I was planning adding a new function chalk.template.instance. And have instance have a constructor so we can do new chalk.instance() and it would just return the template. Would that work?

@sindresorhus sindresorhus mentioned this issue Sep 18, 2018
7 tasks
@shaoron
Copy link

shaoron commented Dec 16, 2018

Let me complete this task

@wuweiweiwu
Copy link

@shaoron please do!

@sindresorhus sindresorhus pinned this issue Dec 26, 2018
@sindresorhus sindresorhus added this to the 3.0.0 milestone Dec 26, 2018
@IssueHuntBot
Copy link

@issuehuntfest has funded $60.00 to this issue. See it on IssueHunt

@IssueHuntBot
Copy link

@tom-sherman has submitted a pull request. See it on IssueHunt

@Qix-
Copy link
Member Author

Qix- commented Jan 26, 2019

@IssueHuntBot don't care about seeing funding status - that's cool and all - but we can already see when people submit PRs. Don't need you to mirror that data, it just causes noise.

cc @Rokt33r

@sindresorhus
Copy link
Member

@IssueHuntBot
Copy link

@sindresorhus has rewarded $54.00 to @tom-sherman. See it on IssueHunt

  • 💰 Total deposit: $60.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $6.00

@sindresorhus sindresorhus unpinned this issue Mar 12, 2019
@issuehunt-oss issuehunt-oss bot added the 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt label May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants