Allow color and label options for jest-diff

[ewanharris]

🚀 Feature Proposal

Allowing passing custom colors/labels to jest-diff for use in the output.

Motivation

I'm using jest-diff in titanium-codemods, which is similar to react-codemod and uses jscodeshift to run transforms against code.

When a users asks for no files to be modified via the --dry-run flag, I output any differences by diffing the modified source, and the original source. The current jest-diff looks a little strange when I do this as the added code is always colored red with +, or green with - depending on which way round I choose. Both of these match the jest ecosystem with snapshots but don't fit for anyone attempting to use jest-diff in a different context.

Showing changes for /Users/eharris/plainalloy/app/lib/appinfo_package.js
- After
+ Before

- `${Ti.App.name} ${Ti.App.version} ${Ti.Platform.name} ${Ti.Platform.version}`
+ `${Ti.App.getName()} ${Ti.App.getVersion()} ${Ti.Platform.getName()} ${Ti.Platform.getVersion()}`

Example

I would imagine this would be implemented as parameter in the diffOptions type for jest-diff. Maybe like below, which fits in with the existing API

const changes = diff(changedSource, original, {
	aAnnotation: 'After',
	aLabel: '+',
	aColor: chalk.green,
	bAnnotation: 'Before',
	bLabel: '-',
	bColor: chalk.red
});

I'm unsure however as to whether I like the aColor/bColor argument. It seems odd to me to tie this specifically to something like chalk, but I guess the typedef is just an argument that takes a string and transforms the string as desired not a specific recommendation.

[ewanharris]

Just a note that I'd also be interested in implementing feature if it sounds good to the maintainers

[jeysal]

Thanks for the proposal, this sounds like a reasonable feature addition to me. We should probably spend some time to design this in a flexible way though.
@pedrottimark Thoughts on this?

[pedrottimark]

Yes, the proposal makes sense. Two details come to my mind in this moment:

• The colors other than green and red are okay? For example, dim for unchanged lines. (This is example of pay off for already having replaced bgGreen and brRed with inverse :)
• Is it okay to assume (restrict) that aLabel and bLabel are single-character strings?

@jeysal Your thoughts on flexible design are welcome, since you also have independent use case.

[SimenB]

I like this idea! Make sure whatever design you come up with also handles word diffing, not just line diffing

[jeysal]

Regarding API design:
I'm wondering if something like aLabel is necessary at all if we have aColor (which I would rather consider something like a "line mapper")? If aColor is a chalk function, it will already wrap the text with leading and trailing characters, so it could add a + in front anyway

[pedrottimark]

@ewanharris With #8572 it looks like this issue has moved to the front burner.

Here is a rough draft of options for you to critique:

export type DiffOptions = {
  aAnnotation?: string;
  aChar?: string; // default '-'
  aColor: Colorer; // default chalk.green
  bAnnotation?: string;
  bChar?: string; // default '+'
  bColor: Colorer; // default chalk.red EDIT corrected the key
  cChar?: string; // default ' '
  cColor: Colorer; // default chalk.dim EDIT corrected the key
  expand?: boolean;
  contextLines?: number;
};

• I changed xLabel to xChar because annotation and label seem like synonyms.
• Is is reasonable to require in API contract that xChar is single character string?
• Is c mnemonic for common lines? I am open to other suggestions.
• Does your tool need to substitute default yellow color for “patch” lines if expand: false

@SimenB The prototype pictures that we will soon make public confirm that chalk.inverse to highlight substrings communicates as clearly for optional colors as for default green and red.

@jeysal To keep from coupling too closely with chalk let’s separate the char from the colorer. Also as we look ahead to data-driven diff when some change lines have common color substrings (for example, key of property whose value has change) it becomes necessary to call the change colorer on the preceding and following substring, so that dim substring matches common lines instead of becoming almost illegible dim green or dim red, or whatever custom color.

[pedrottimark]

@ewanharris

• Do your codemods call jest-diff with non-default values of contextLines and expand?

• What do you think about option to display number of changed lines following annotations?

- Before  -1
+ After   +1

- `${Ti.App.name} ${Ti.App.version} ${Ti.Platform.name} ${Ti.Platform.version}`
+ `${Ti.App.getName()} ${Ti.App.getVersion()} ${Ti.Platform.getName()} ${Ti.Platform.getVersion()}`

• You might be interested in getDiffString added in expect: Highlight substring differences when matcher fails, part 2 #8528 for next minor version of jest-diff

[jeysal]

• Is is reasonable to require in API contract that xChar is single character string?

I think anything "character" or "char" might be dangerous in a Unicode context because it could be misunderstood as "byte", which is often the same thing as "char"

[pedrottimark]

What do you think about aSymbol bSymbol cSymbol and the API contract is same width in monospaced font?

An example that comes to mind for third-party dependent is aSymbol: '<' and bSymbol: '>'

[ewanharris]

@pedrottimark, sorry for my radio silence here. I'll try and find some time in the next few days to respond

[ewanharris]

• Do your codemods call jest-diff with non-default values of contextLines and expand?
  • I'm passing in expand: false and contextLines: true, seeing as I'm passing these in I'm guessing they're not the default
• What do you think about option to display number of changed lines following annotations?
  • That looks ace, it makes it really easy to glance and see how many changes there are easily
• You might be interested in getDiffString added in expect: Highlight substring differences when matcher fails, part 2 #8528 for next minor version of jest-diff
  • I'll take a look, that does look pretty good for my usecase

Other than the comment from @jeysal that interface looks good to me. I could see some confusion over the usage of c maybe the full word common could be used instead?

[pedrottimark]

@ewanharris

• Yes, I agree common is better than c as prefix and am open to other terms that you suggest
• For your info, the contextLines optional property has number type with default value 5 so if that works for you, then you can omit the property, because it is ignoring boolean value true