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

npm diff #1319

Merged
merged 1 commit into from
Jan 28, 2021
Merged

npm diff #1319

merged 1 commit into from
Jan 28, 2021

Conversation

ruyadorno
Copy link
Contributor

@ruyadorno ruyadorno commented May 17, 2020

ℹ️ [Update]: I also wrote a blog post showcasing usage of the npm diff command in a more lightweight read format, in case someone lands here looking to learn more about it.

The registry diff command

Introduce a new npm diff command for comparing published versions across the npm registry, complementing the experience from npm outdated and npm audit for package consumers and providing a new workflow for package authors to quickly assess the delta between local changes and a specific published version of a package.

What's in here

  • git-diff-unified-compatible output so that users can plug in any existing tools they already have as part of their workflow and enables IDE/tools to also easily integrate it.
  • npm diff with no params, useful for package authors, returns a diff between current files and the latest published version (or customizable tag).
  • npm diff --diff=<spec> compares a current installed package to a spec defined by the user.
  • npm diff --diff=<spec-a> --diff=<spec-b> compares two diff published versions of a package.
  • npm diff --diff=<version-a> --diff=<version-b> version comparison shorthand for a current package.
  • npm diff [<paths>...] positional args filters what files to use, accepts globs and dir names.

Supports some of the most popular git diff options:

git diff optsnpm diff equivalent
-w/--ignore-all-space--diff-ignore-all-space
-U/--unified--diff-unified
--name-only--diff-name-only
-a/text--diff-text
--no-prefix--diff-no-prefix
--src-prefix--diff-src-prefix
--dst-prefix--diff-dst-prefix

@ljharb
Copy link
Contributor

ljharb commented May 17, 2020

Prior art: https://npmjs.com/lockfile-diff

@ruyadorno
Copy link
Contributor Author

Prior art: https://npmjs.com/lockfile-diff

A more similar userland solution I found was this script: https://github.com/juliangruber/npm-diff

@ljharb
Copy link
Contributor

ljharb commented May 18, 2020

Before landing this, it'd be great to discuss it on an RFC call to make sure everyone's use cases are considered.

@ruyadorno
Copy link
Contributor Author

Before landing this, it'd be great to discuss it on an RFC call to make sure everyone's use cases are considered.

for sure! 😄

@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature labels Sep 1, 2020
@darcyclarke darcyclarke added Priority Backlog a "backlogged" item that will be tracked in a Project Board and removed Priority Backlog a "backlogged" item that will be tracked in a Project Board labels Sep 25, 2020
@ruyadorno ruyadorno changed the base branch from release/v7.0.0-beta to latest January 20, 2021 05:37
@ruyadorno ruyadorno marked this pull request as ready for review January 20, 2021 05:52
@ruyadorno ruyadorno requested a review from a team as a code owner January 20, 2021 05:52
@ruyadorno ruyadorno added the release: next These items should be addressed in the next release label Jan 20, 2021
@ruyadorno ruyadorno added this to the OSS - Sprint 23 milestone Jan 20, 2021
@npm-deploy-user
Copy link

npm-deploy-user commented Jan 20, 2021

found 2 benchmarks with statistically significant performance improvements

  • app-large: cache-only:legacy-peer-deps
  • app-medium: cache-only
timing results
app-large clean lock-only cache-only cache-only
legacy-peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@7 45.446 ±0.45 29.736 ±0.49 28.726 ±0.05 28.028 ±0.18 2.324 ±0.02 2.355 ±0.01 1.667 ±0.01 12.127 ±0.18 1.660 ±0.05 3.523 ±0.02
#1319 44.397 ±1.22 30.483 ±0.18 26.612 ±1.02 25.034 ±0.16 2.356 ±0.03 2.361 ±0.01 1.680 ±0.02 11.730 ±0.04 1.669 ±0.03 3.493 ±0.08
app-medium clean lock-only cache-only cache-only
legacy-peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@7 33.346 ±0.60 21.273 ±0.01 20.534 ±0.47 19.696 ±0.28 2.212 ±0.04 2.196 ±0.04 1.560 ±0.00 7.639 ±0.00 1.541 ±0.01 3.029 ±0.19
#1319 30.686 ±0.49 21.466 ±0.04 17.633 ±0.10 17.840 ±0.51 2.217 ±0.03 2.216 ±0.00 1.572 ±0.01 7.589 ±0.11 1.572 ±0.01 2.897 ±0.00

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

It’d be great if this supported all the arguments git diff did - eg, i use --name-only constantly when i want filenames instead of contents, and i use the pager config and the diff tool config frequently. Additionally, the -w option to suppress whitespace diffs (there’s a number of others)

i think it would be very confusing for one’s muscle memory if the command is called “diff” and doesn’t do what diff/git diff does. Thoughts?

@ruyadorno
Copy link
Contributor Author

@ljharb it does support all these options you mentioned! Thanks to feedback from you, @bnb and @coreyfarrell in the original RFC 😄 You can check the full list of options in the docs here: https://github.com/npm/cli/pull/1319/files#diff-81ea63c7baccd94f458fdc214180a305932547f44f5df499a6363d1cc29c8e5bR89

Now that said, have you actually poked at all git diff options to see what that implies? https://git-scm.com/docs/git-diff#_options 😰 I think using the old analogy saying that "git diff is the operational system of diffs" is very valid here.

My proposal instead is that we start with these popular/essential set of options and we can work from there, continuing to incrementally add new configurations as we identify them as being essential over time.

Hope that makes sense and thanks again with all the help! It has been very appreciated 😊

@ljharb
Copy link
Contributor

ljharb commented Jan 20, 2021

Beautiful, thanks :-) certainly i don’t know nor have used all the diff options, and until i discover one missing will likely be perfectly content in assuming they all work.

@ruyadorno
Copy link
Contributor Author

One note specifically on pager though, I think that should be a follow up PR / backlog item as it may also be reused across different npm commands and it's not a blocker on shipping npm diff.

I acknowledge that from a UX pov I would love to have it from day1 but it's a tradeoff I'm willing to make in order for this to be shipped (after so many months 🙃 ).

Copy link
Contributor

@darcyclarke darcyclarke left a comment

Choose a reason for hiding this comment

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

Need to change config values as theres some collisions/issues here.

@darcyclarke darcyclarke added release: next These items should be addressed in the next release Needs Review and removed release: next These items should be addressed in the next release labels Jan 21, 2021
@ruyadorno
Copy link
Contributor Author

Small update on this, after reviewing this thoroughly with @isaacs we decided to adjust options to be more npm-cli friendly. In practice that means we'll be prefixing the original git options (e.g: --name-only becomes --diff-name-only) and the specs to compare are going to be named arguments instead of positional (in order to avoid usage of the -- separator to disambiguate paths and specs).

Will follow up with these updates soon 😊

@ljharb
Copy link
Contributor

ljharb commented Jan 22, 2021

@ruyadorno the prefix makes sense, but why would you need the -- operator? i'd expect npm diff a b to work whether a and b were paths or specs.

@ruyadorno
Copy link
Contributor Author

@ljharb it was needed in order to enable filtering by filepaths, since both specs-to-compare and paths-to-filter are positional arguments. That was also a syntax choice that I was trying to inherit from git (since in git both commitish and paths are positional arguments they have to use a -- separator to disambiguate some cases).

In git diff it means that if you have a branch named latest and also a file named latest in your git repo and you try to run git diff latest you get the following result:

$ git diff latest
fatal: ambiguous argument 'latest': both revision and filename
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

Back to npm diff, originally I was trying to copy the syntax style by having both comparator-specs AND filter options be positional arguments, since in npm a file system path is ALSO a valid spec. That becomes a problem given that the comparator specs are also optional, e.g: npm diff, npm diff <spec-a> and npm diff <spec-a> <spec-b> are all valid.

Then what about if I want to use npm diff (no args) which compares the local file system against the latest published version of the cwd package but I ALSO want to filter out by a specific filepath to filter? With no way to disambiguate using just npm diff <filepath> might be interpreted as if I'm actually using the npm diff <spec-a> syntax which is the one meant to be a companion to npm outdated and more useful to package consumers - so the way to disambiguate those and use the standard npm diff while filtering filepaths would be to provide the -- separator in between them, e.g: npm diff -- lib/*.

Sorry about the lenght of it but this is the very long answer as to why we would need the -- 😅

@ljharb
Copy link
Contributor

ljharb commented Jan 22, 2021

Given that's already how git diff works, it seems like it'd be more confusing to deviate from it.

ruyadorno added a commit to npm/libnpmdiff that referenced this pull request Jan 27, 2021
Syncs APIs with changes proposed in the `npm diff` PR.

ref: npm/cli#1319
@ruyadorno
Copy link
Contributor Author

ruyadorno commented Jan 27, 2021

Heads up @ljharb and anyone else interested in this, after a few rounds of reviews with the team and specially feedback from @isaacs we landed on using translated config names to avoid colliding names (such as the --no-prefix option from git with the npm cli --prefix).

We also changed the syntax to define specs using a named option --diff so that we don't need to add any extra semantics with the -- separator in order to filter by file names.

Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

Handful of relatively minor changes. Reviewed the docs and test much more carefully than the code, but since the tests pass, this is fine. Suggested changes can be made later (or maybe even pushed back on in some cases, idk).

docs/content/commands/npm-diff.md Show resolved Hide resolved
docs/content/commands/npm-diff.md Outdated Show resolved Hide resolved
docs/content/commands/npm-diff.md Outdated Show resolved Hide resolved
docs/content/commands/npm-diff.md Outdated Show resolved Hide resolved
docs/content/commands/npm-diff.md Outdated Show resolved Hide resolved
lib/diff.js Outdated Show resolved Hide resolved
lib/utils/config.js Outdated Show resolved Hide resolved
lib/utils/flat-options.js Show resolved Hide resolved
test/lib/diff.js Outdated Show resolved Hide resolved
test/lib/diff.js Show resolved Hide resolved
lib/diff.js Outdated Show resolved Hide resolved
lib/diff.js Outdated Show resolved Hide resolved
- As proposed in RFC: npm/rfcs#144

PR-URL: #1319
Credit: @ruyadorno
Close: #1319
Reviewed-by: @isaacs
@nlf nlf changed the base branch from latest to release/v7.5.0 January 28, 2021 20:50
@nlf nlf merged commit d011266 into release/v7.5.0 Jan 28, 2021
@nlf nlf mentioned this pull request Jan 28, 2021
@nlf nlf deleted the ruyadorno/npm-diff branch March 28, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants