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

jest-diff CLI #7781

Closed
wants to merge 4 commits into from
Closed

jest-diff CLI #7781

wants to merge 4 commits into from

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Feb 2, 2019

Summary

jest-diff is a nice utility for comparing large JSON structures. I've written node scripts like

const {readFileSync} = require('fs')
const diff = require('jest-diff')
const a = JSON.parse(readFileSync('/tmp/a'))
const b = JSON.parse(readFileSync('/tmp/b'))
console.log(diff(a, b))

quite a few times to compare e.g. HTTP response bodies.
This PR adds a CLI to jest-diff that can be used after installing jest-diff globally (or, if installed locally, e.g. in a project's shell scripts).

It is sort of a minimum viable implementation, it would be conceivable to add e.g. support for command-line flags that control DiffOptions in the future.
It would also be conceivable to give other packages this treatment if they could be useful as a CLI tool, e.g. pretty-format.

cc @pedrottimark had this lying around semi-finished for some time now - this is the thing that lead me to #7605 😄

Test plan

CLI e2e tests included.
jest-diff script added for convenient manual testing.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

This is great! :) Left some nits to address. Also adding documentation to jest-diff's README would be a plus

packages/jest-diff/src/cli/index.js Outdated Show resolved Hide resolved
packages/jest-diff/src/cli/index.js Outdated Show resolved Hide resolved
packages/jest-diff/src/cli/index.js Outdated Show resolved Hide resolved
packages/jest-diff/src/cli/index.js Outdated Show resolved Hide resolved
@pedrottimark
Copy link
Contributor

Yes, I have also written script like your example :)

By the way, see changed-lines package

What do you think about comment on chat by @SimenB to do it as jest-community/jest-diff-cli

@jeysal
Copy link
Contributor Author

jeysal commented Feb 3, 2019

Also adding documentation to jest-diff's README would be a plus

There is no jest-diff README :D
Thought Jest platform docs were the best place to mention it, any other places where it might be worth a mention?

@jeysal
Copy link
Contributor Author

jeysal commented Feb 3, 2019

What do you think about comment on chat by @SimenB to do it as jest-community/jest-diff-cli

Sindre Sorhus would be proud :D
I can see both pros and cons of splitting off CLI packages in general - I don't feel strongly about this, willing to go with what the majority thinks is right or whatever 😅

I'm not sure about moving it to community. I think there's some merit in keeping it tied to Jest releases. Pretty much every feature of jest-diff will also be a feature of jest-diff-cli and every breaking change of jest-diff also a breaking change of jest-diff-cli. Keeping the community package up to date would probably a bit of a hassle and I think the burden of maintaining this in the main repo is moderate.

@thymikee thymikee dismissed their stale review February 3, 2019 14:40

feedback addressed

@pedrottimark
Copy link
Contributor

Yes, what you say about releasing consistent versions from Jest monorepo makes sense.

I remember that core versus cli came up for ESLint but forget the details. @SimenB, for the public record (as Nicholas would admonish us) can you summarize precedents from other projects?

Regardless of decision about packaging, I am happy for another known use case for jest-diff

  • find or prevent other too tight coupling (like what you fixed) between it and expect package
  • remove excuse for lack of README :(

@SimenB
Copy link
Member

SimenB commented Feb 3, 2019

I'm happy for the CLI to live in the monorepo, fwiw. It's keeping it as a single package I'm against.

can you summarize precedents from other projects?

https://github.com/sindresorhus?utf8=%E2%9C%93&tab=repositories&q=-cli&type=source&language=

😅

My thinking is that, unless a tool's primary use is to be run through a CLI, it shouldn't bundle a CLI by default.
Keeping it separate also means keeping the API of the implementation module cleaner as the barrier to accessing private APIs from the CLI is higher, when it's not require('../lib/thing');. Potentially creating a better/more powerful API for other consumers of the implementation as well.


Regardless of decision about packaging, I am happy for another known use case for jest-diff

100% agree


// eslint-disable-next-line import/default
import jestDiff from '../';
import {NO_DIFF_MESSAGE} from '../constants';
Copy link
Member

Choose a reason for hiding this comment

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

This is IMO an example of where a separate CLI would force a better API.

It's imported to allow the CLI to remove a message that shouldn't really be in a diffing algorithm. Instead it should probably be an option (or structured return values, or something else entirely), but since we have access to private stuff, it's more natural to just import it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it really bugged me writing this that the return values of jest-diff are so weird 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, those constants :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we fix this without a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, adding an option which changes the returned thing (from a string to something structured) should be fine in a minor (as long as it's opt-in). Might be a bit painful WRT flow types but probably fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fool around with some non-breaking API designs.
Probably on the weekend. So much work, so little time for my Jest todo list :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, mee too :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the null return values for asymmetric matchers: Would it be fine to just treat them like "no difference"? The reason they were originally introduced was #8146 - in that case, you would get the "No visual difference" message as well. Sooner or later we'll have to rethink jest-diff (and expect and pretty-format) in light of #6184 , but for now it would be good to have just "differs, here's the string" and "does not differ (visually)" as returns values so that we can come up with a clean and simple API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Actually, what's now SIMILAR_MESSAGE will need to have a special case in the return value too :/ )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we should return something like {differs: true, message: string, toJsonIgnored: boolean} | {differs: false}

@jeysal
Copy link
Contributor Author

jeysal commented Nov 12, 2019

I'll close this for now since I don't think I'll have time to make the required changes to finish this anytime soon and it's not high priority. If anyone wants to pick this up, feel free. Otherwise it'll be somewhere on my todo list 😅

@jeysal jeysal closed this Nov 12, 2019
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants