feat: enable themes (#852) #862

mdonnalley · 2023-11-09T19:45:49Z

Merge #852 to main

* feat: create type for storing theme colors * feat: include color and @types/color, and simiplify theme schema * feat: set theme's default colors to white * feat: set FORCE_COLOR to 0||3 to follow the NO_COLOR manifest * feat: create DEFAULT_THEME to store default colors * revert: remove NO_COLOR manifest * feat: add new theme variables to style $, flag, and flag options * feat: add color to section headers * feat: configure default colors * feat: add colors for bin, command summary and version * feat: topics, commands, bin, version, sections, dollar sign are colorized * feat: configure default colors * feat: add feature flag to enabled/disable theme * feat: add colorize function to simplify the way colors are added to strings * feat: change all chalk.hex calls to colorize * feat: configure OCLIF_ENABLE_THEME to have precence over PJSON prop * feat: all theme colors are optional * fix: error TS2322: Type 'string' is not assignable to type 'boolean' * fix: error TS2345: arg of type 'Color<ColorParam>|undefined' not assignable to 'Color<ColorParam>' * fix: runtime error TypeError: color.hex is not a function * fix: this.pjson.oclif.enableTheme was not being evaluated when OCLIF_ENABLE_THEME was unset * chore(deps): update yarn.lock * refactor: simplified code removing OCLIF_ENABLE_THEME constant * fix: command summary was not changing its color when running the root command * feat: theme is now read from ~/config/<CLI>/theme.json if one exists * fix: add colors to other ARGUMENTS, EXAMPLES, DESCRIPTION, FLAGS DESCRIPTIONS sections * feat: add color token for aliases * fix(test): change template strings to avoid expected results printing bin twice * fix(test): add a missing parenthesis back so that all options are wraped by () * fix(test): remove single quotes wraping default flag values * feat: add test:debug script to ease debuging * refactor: add a constant with all possible THEME_KEYS * test: add tests to prove parsing untyped json object with color strings work * chore(package): add new scripts to ease development while writing tests * revert: remove theme from PJSON because a theme will be loaded from config_dir/<CLI>/theme.json * feat: add scopedEnvVarBoolean because scopedEnvVarTrue does not consider unset env vars * feat(test): add tests to prove the behavior that enables theme * refactor: replace all scopedEnvVarTrue by scopedEnvVarBoolean, which considers unset env variables * test: add tests to prove the enableTheme behavior * refactor: simplify parseTheme method * chore(package): remove localhost:4873 from lock file * revert: rever scopedEnvVarBoolean back to scopedEnvVarTrue * test: add tests to prove when this.theme is set * feat: ensure colorize returns string only * test: add tests to prove colorize works as expected * revert: rollback scopedEnvVarTrue as it was before * refactor: move parseTheme to src/util/util.js * refactor: make Theme type dinamically based on the values of THEME_KEYS at runtime * fix: err TS1259: Module /@types/color/index can only be default-imported using 'esModuleInterop' * revert: remove enableTheme from pjson because we don't want to cli devs to force users to use theme * revert: remove undefined as a return type for scopedEnvVarTrue * refactor: remove config.enableTheme

AllanOricil · 2023-11-10T00:58:34Z

@mdonnalley
I forgot to change http to https on those dependency entries in the log file.
Merge this fix in your branch: #869

AllanOricil · 2023-11-13T02:23:01Z

@mdonnalley when you release this feature could you please make some sort of "promotion" with my name? Now that I have time, I'm working on open source projects that I have interest in order to increase my chances of leaving this s*** country when my parents are gone. If you can't do it, there is no problem. Thank you in advance.

mdonnalley · 2023-11-13T15:40:39Z

when you release this feature could you please make some sort of "promotion" with my name?

@AllanOricil No problem. You're name will be in the commit history and we'll mention your name in the sf release notes once we integrate this feature there. Does that work for you?

)

AllanOricil · 2023-11-13T16:47:47Z

@mdonnalley I would like to also have my name somewhere on those posts/videos that Salesforce usually release to notify devs about new features

…y/852

AllanOricil · 2023-11-13T22:32:14Z

@mdonnalley you removed the test that tests that a certain key is present in the parsed theme. If a developer remove alias from Theme all tests will pass and we wont be able to see a regression. Those tests were there to prove that the key can not be removed from Theme without causing a regression

mdonnalley · 2023-11-13T22:37:27Z

@AllanOricil Are you referring to these tests?

If so, I don't see what value that provides that this test doesn't. They're the exact same tests except one has more properties in the parsed theme

AllanOricil · 2023-11-13T22:43:24Z

@AllanOricil Are you referring to these tests?

If so, I don't see what value that provides that this test doesn't. They're the exact same tests except one has more properties in the parsed theme

@mdonnalley in the first, if you remove alias from Theme, the code will not compile with a type error. In the second, because there is no ref to the key name, the code compiles. The benefit is that a dev will see that removing alias from Theme causes a regression and he can must not do it. Or Im saying nonsense. Let me test it. one moment.

EDIT: I was actually wrong. The code does compile, but 1 test fail when removing a key from THEME_KEYS, which is good to tell developers that a regression occurred and therefore a key must not be removed. Sorry for the mistake.

AllanOricil · 2023-11-13T22:49:04Z

@mdonnalley look, when I removed alias from THEME_KEYS, 1 single test failed so that a developer can know exactly the root cause of the problem. It is like if it was a real world experiment, you change 1 variable, it changes 1 single behaviour.

AllanOricil · 2023-11-13T22:58:37Z

@mdonnalley Yes, with the current code there is no way to know if another developer deleted any of the mandatory Theme keys because there is no tests to test the which keys are valid in the Theme specification.

AllanOricil · 2023-11-13T22:18:27Z

src/interfaces/theme.ts

+export const STANDARD_CHALK = [
+  'white',
+  'black',
+  'blue',
+  'yellow',
+  'green',
+  'red',
+  'magenta',
+  'cyan',
+  'gray',
+  'blackBright',
+  'redBright',
+  'greenBright',
+  'yellowBright',
+  'blueBright',
+  'magentaBright',
+  'cyanBright',
+  'whiteBright',
+  'bgBlack',
+  'bgRed',
+  'bgGreen',
+  'bgYellow',
+  'bgBlue',
+  'bgMagenta',
+  'bgCyan',
+  'bgWhite',
+  'bgGray',
+  'bgBlackBright',
+  'bgRedBright',
+  'bgGreenBright',
+  'bgYellowBright',
+  'bgBlueBright',
+  'bgMagentaBright',
+  'bgCyanBright',
+  'bgWhiteBright',
+] as const
+
+export type StandardChalk = (typeof STANDARD_CHALK)[number]


I was able to pass 'red' to color without a problem . What happens if this is not in the code?

Never mind, I tested and there are some colors, like 'blackBright' that can't be parsed by the color package. Really good finding!

For those who want to see a proof

AllanOricil · 2023-11-14T14:01:12Z

test/util/theme.test.ts

+      alias: 'cyan',
+      bin: 'cyan',
+      command: 'cyan',
+      commandSummary: 'cyan',
+      dollarSign: 'cyan',
+      flag: 'cyan',
+      flagDefaultValue: 'cyan',
+      flagOptions: 'cyan',
+      flagRequired: 'cyan',
+      flagSeparator: 'cyan',
+      sectionDescription: 'cyan',
+      sectionHeader: 'cyan',
+      topic: 'cyan',
+      version: 'cyan',
+    }


cyan is supported by the colors package. Use a color, like blackBright, that would fail to be parsed without STANDARD_CHALK in order to make this a non false positive test.

Here is a test where I discovered that blackBright isn't supported by the color package

AllanOricil · 2023-11-14T14:12:20Z

src/util/theme.ts

+}
+
+export function colorize(color: Color | StandardChalk | undefined, text: string): string {
+  if (isStandardChalk(color)) return standardChalk(color, text)


You could remove this function and just use return chalk[color](text)

Using a theme with standard chalk colors

AllanOricil · 2023-11-14T14:20:18Z

src/util/theme.ts

+export function parseTheme(untypedTheme: Record<string, string>): Theme {
+  return Object.fromEntries(Object.entries(untypedTheme).map(([key, value]) => [key, getColor(value)]))


the theme feature should not waste time or memory storing tokens its spec does not support (strict). Its spec supports these tokens:

export const THEME_KEYS = [ 'alias', 'bin', 'command', 'commandSummary', 'dollarSign', 'flag', 'flagDefaultValue', 'flagOptions', 'flagRequired', 'flagSeparator', 'flagType', 'sectionDescription', 'sectionHeader', 'topic', 'version', ]

Then you have to add these tests back because they prove the independency and usage of each token of Theme Spec. We need it so that when tokens are changed we know exactly the regressions the change made. For example, if I remove the alias token and run the tests, I will see that its test failed, and this will show me a regression:

@AllanOricil I want to leave the possibility for people to be able to use the theme beyond just the help output.

For instance, someone might want to have a warningMsg color that gets used on all their warning messages.

export default class MyCommand extends Command { public async run(): Promise<void> { this.logToStderr(colorize(this.config.theme.warningMsg, 'hello world')) } }

@mdonnalley we would change logToStderr to the following piece of code so that they can keep using this.logToStderr without updating their code. The theme.json is what controls the color changing.

public static logToStderr(format?: string, ...args: string[]): void { write.stderr(colorize(this.config?.theme?.warningMsg, utilFormat(format, ...args) + '\n')) }

and a new token would be added to the Theme Spec

const THEME_KEYS = { ..., "warningMsg" }

If this.config?.theme?.warningMsg does not exist, this.logToStderr will output what is currently outputing. It would not require any code updates in CLIs using @oclif/core.

If CLI developers want to overwrite what the theme.json says, they can do this

export default class MyCommand extends Command { public async run(): Promise<void> { this.config.theme.warningMsg = getColor('brightBlack') this.logToStderr('hello world') } }

and for logToStderr, a better Theme Key is stderrMessage,

public static logToStderr(format?: string, ...args: string[]): void { write.stderr(colorize(this.config?.theme?.stderrMessage, utilFormat(format, ...args) + '\n')) }

It's a very poor assumption that everyone would be using logToStderr to only log warning messages. But my point was that by allowing any key in themes.json we give CLI developers the ability to create a more extensive theme for their users. I have no interest in standardizing themes for every single oclif CLI

Going back to what I said earlier, there are no tests to "prove" the Theme Spec the Core Specifies...

If the core is using certain keys everywhere in its code, there must exist tests to prove each single one of these keys....

so that when someone deletes or changes the key, a test failure happens to show the Regression.

ok, thanks for adding a test back. Now it is working again as I specified.

That's what this is: https://github.com/oclif/core/pull/862/files#diff-6bef7aac23bf8d9e00a172402d3b443a7a6e1834198135027ce593fa877e8f73R116

AllanOricil · 2023-11-14T17:53:28Z

src/interfaces/theme.ts

+export type ThemeKey = (typeof THEME_KEYS)[number]
+
+export type Theme = {
+  [key: string | ThemeKey]: Color | StandardChalk | undefined


Ok, I see a use case for this. CLI developers can extend the oclif's Theme Spec with their own theme specific keys so that plugins built with their own CLI API can take advantage of their own Theme Spec, which is just a combination of Oclif + CLI Theme Specs. Makes sense now.

Shouldn't Theme be a interface to allow it to be extensible in typescript world?

AllanOricil · 2023-11-14T19:26:29Z

I don't see anything else I would change, so I've approved it. And I can't see why tests are failing to help fix it. I will go do something else.

AllanOricil · 2023-11-16T17:50:29Z

src/interfaces/theme.ts

+export const STANDARD_CHALK = [
+  'white',
+  'black',
+  'blue',
+  'yellow',
+  'green',
+  'red',
+  'magenta',
+  'cyan',
+  'gray',
+  'blackBright',
+  'redBright',
+  'greenBright',
+  'yellowBright',
+  'blueBright',
+  'magentaBright',
+  'cyanBright',
+  'whiteBright',
+  'bgBlack',
+  'bgRed',
+  'bgGreen',
+  'bgYellow',
+  'bgBlue',
+  'bgMagenta',
+  'bgCyan',
+  'bgWhite',
+  'bgGray',
+  'bgBlackBright',
+  'bgRedBright',
+  'bgGreenBright',
+  'bgYellowBright',
+  'bgBlueBright',
+  'bgMagentaBright',
+  'bgCyanBright',
+  'bgWhiteBright',
+  'bold',
+  'underline',
+  'dim',
+  'italic',
+  'strikethrough',
+] as const


Can't these exports work? I did not check their values, but by looking at the variable names it seems to be what we need

https://github.com/chalk/chalk/blob/main/source/index.js#L207-L218

Never mind. That is for v5. V4 does not have those exports, unfortunately

@mdonnalley

In v4 I think you could use

Object.getOwnPropertyNames(chalk)

edited: we don't have access to Chalk class, but it is possible to use an instance of it

Based on the following, all styles are direct props from the chalk prototype:

https://github.com/chalk/chalk/blob/95d74cbe8d3df3674dec1445a4608d3288d8b73c/source/index.js#L222

AllanOricil · 2023-11-16T20:34:48Z

src/config/config.ts

+  public async loadThemes(): Promise<{
+    file: string
+    activeTheme: Theme | undefined
+    themes: Themes | undefined
+  }> {
+    const themesFile = this.pjson.oclif.themesFile
+      ? resolve(this.root, this.pjson.oclif.themesFile)
+      : resolve(this.configDir, 'themes.json')
+    const themes = await safeReadJson<Themes>(themesFile)
+    const activeTheme = themes ? parseTheme(themes) : undefined
+    return {
+      activeTheme,
+      file: themesFile,
+      themes,
+    }
+  }
+


I don't understand why loading all themes in memory if only one is used at a time

will be used by https://github.com/oclif/plugin-theme/pull/7

It does not answer the question. What is the use case to have all themes in memory?

AllanOricil · 2023-11-16T21:25:54Z

I envisioned and started this feature and you took it over and started to make decisions without discussing with me. This is extremely unprofessional and it will be registered here for other people to see.

mdonnalley · 2023-11-16T21:54:56Z

I envisioned and started this feature and you took it over and started to make decisions without discussing with me. This is extremely unprofessional and it will be registered here for other people to see.

It's an open PR - no decisions have been made. Your feedback is welcome, including on the plugin.

Your reviews and comments on this PR have been incredibly helpful thus far - I'm sure you've noticed that many of the commits here are addressing your feedback. From my perspective, this has been a collaborative effort.

The current implementation in this PR and the plugin-theme PR was discussed here. As Cristian mentioned in his comment, we're not willing to take on the complexity of having themes be installable via npm so we're looking for a more file-based solution. We'd love if you could add your thoughts there so that we can starting working towards a solution that we're all happy with

AllanOricil · 2023-11-18T16:39:35Z

Hi @mdonnalley sorry for what I said earlier. From time to time I have stupid emotional outages and I end up saying stupid things because my mind starts to think about things that doesnt exist. For example, I think that Im losing something and being left behind, or I get envy, again for no reason. I take some medication but it just doesn't work well sometimes. You can continue this and follow your ideas. You actually did most of the good decisions. I hope to use it when it is finished. Again, sorry.

salesforce-cla bot added the cla:signed label Nov 9, 2023

mdonnalley added 2 commits November 9, 2023 12:50

fix: https in yarn.lock

418d124

feat: use chalk for standard colors

52f2240

AllanOricil mentioned this pull request Nov 12, 2023

fix: change http to https in the lock file #869

Merged

fix: change http to https in order to pass lockfile-check workflow (#869

8cd05e4

)

mdonnalley added 6 commits November 13, 2023 09:48

fix: streamline coloring

295dd30

Merge branch 'mdonnalley/852' of github.com:oclif/core into mdonnalle…

a21b119

…y/852

Merge branch 'main' into mdonnalley/852

d2c1696

fix: remove standardized id from usage string

cd746a6

test: provide id to mocked command class

0db78d3

Merge branch 'main' into mdonnalley/852

ccad43c

AllanOricil reviewed Nov 14, 2023

View reviewed changes

AllanOricil suggested changes Nov 14, 2023

View reviewed changes

chore: code review and clean up

1a4c32b

mdonnalley force-pushed the mdonnalley/852 branch from 06d5400 to 1a4c32b Compare November 14, 2023 17:48

AllanOricil approved these changes Nov 14, 2023

View reviewed changes

fix: support more chalk standards

0194eaa

AllanOricil reviewed Nov 16, 2023

View reviewed changes

fix: prepare for plugin-theme

31604f8

mdonnalley and others added 4 commits November 16, 2023 12:50

chore: add jsdoc to ux.colorize

80cbda2

chore(release): 3.11.1-dev.0 [skip ci]

10fccf8

fix: always convert to hex

171c983

chore(release): 3.11.1-dev.1 [skip ci]

78bdacc

AllanOricil reviewed Nov 16, 2023

View reviewed changes

mdonnalley mentioned this pull request Nov 16, 2023

feat: document themes oclif/oclif.github.io#219

Merged

chore: remove unused type

0886fbf

chore: revert to single theme.json

cdff2ad

mdonnalley merged commit da2bd5b into main Nov 20, 2023
35 of 37 checks passed

mdonnalley deleted the mdonnalley/852 branch November 20, 2023 20:19

		export function parseTheme(untypedTheme: Record<string, string>): Theme {
		return Object.fromEntries(Object.entries(untypedTheme).map(([key, value]) => [key, getColor(value)]))

feat: enable themes (#852) #862

feat: enable themes (#852) #862

Conversation

mdonnalley commented Nov 9, 2023 • edited Loading

AllanOricil commented Nov 10, 2023 • edited Loading

AllanOricil commented Nov 13, 2023

mdonnalley commented Nov 13, 2023

AllanOricil commented Nov 13, 2023

AllanOricil commented Nov 13, 2023 • edited Loading

mdonnalley commented Nov 13, 2023

AllanOricil commented Nov 13, 2023 • edited Loading

AllanOricil commented Nov 13, 2023

AllanOricil commented Nov 13, 2023

Choose a reason for hiding this comment

AllanOricil Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanOricil Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanOricil Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanOricil Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanOricil commented Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanOricil Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanOricil commented Nov 16, 2023

mdonnalley commented Nov 16, 2023

AllanOricil commented Nov 18, 2023

mdonnalley commented Nov 9, 2023 •

edited

Loading

AllanOricil commented Nov 10, 2023 •

edited

Loading

AllanOricil commented Nov 13, 2023 •

edited

Loading

AllanOricil commented Nov 13, 2023 •

edited

Loading

AllanOricil Nov 14, 2023 •

edited

Loading

AllanOricil Nov 14, 2023 •

edited

Loading

AllanOricil Nov 14, 2023 •

edited

Loading

AllanOricil Nov 14, 2023 •

edited

Loading

AllanOricil commented Nov 14, 2023 •

edited

Loading

AllanOricil Nov 16, 2023 •

edited

Loading