-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
Migrate to colorette 2.0.x #2956
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,6 +1,22 @@ | ||||
let colorette = null; | ||||
|
||||
module.exports = { | ||||
|
||||
get colors() { | ||||
return require("colorette"); | ||||
if (!colorette || colorette.options.changed) { | ||||
const col = require('colorette'); | ||||
const options = { | ||||
changed: false, | ||||
enabled: colorette? colorette.options.enabled : true, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems some weird, colorette already has logic for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See existing code at
From my understanding this is an existing solution to trigger changes in the behaviour of instance, returned by getter. Alternatively, we can expose a method that would instantly recreate instance, stored here, to avoid this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should rewrite logic, because we already cache module, we can keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. caching module is not enough, we need to store initialized coloretto instance as well |
||||
} | ||||
|
||||
colorette = { | ||||
...col.createColors({ useColor: options.enabled }), | ||||
options, | ||||
} | ||||
} | ||||
|
||||
return colorette; | ||||
}, | ||||
|
||||
get levenshtein() { | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid recreating colorette instance on every getter call. We could simply import default colouring operations, but these would default to always on and ignore configuration overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to classify the always-color behavior as a bug after all (jorgebucaran/colorette#72). If you don't really care about the new
createColors
API, you may continue using Colorette as you were before, just upgrade to2.0.5
or later.