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

RFC: Add npm diff #144

Merged
merged 1 commit into from Feb 18, 2021
Merged

RFC: Add npm diff #144

merged 1 commit into from Feb 18, 2021

Conversation

ruyadorno
Copy link
Contributor

@ruyadorno ruyadorno commented May 19, 2020

@ljharb
Copy link
Contributor

ljharb commented May 19, 2020

Interesting - this is diffing between the file contents, but i'd also want to know the theoretical lockfile diff - iow, what transitive dep name/version changes have happened across that span.

@ruyadorno ruyadorno added Agenda will be discussed at the Open RFC call semver:minor new backwards-compatible feature labels May 19, 2020
accepted/0000-npm-diff.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Contributor

ljharb commented May 20, 2020

I also asked during the RFC call about whether the diff should be from "current to wanted" by default rather than "current to latest", so as to avoid breaking changes (matching what npm update does by default)

@ruyadorno
Copy link
Contributor Author

btw we actually have a working prototype over at npm/cli#1319

@ruyadorno ruyadorno removed the Agenda will be discussed at the Open RFC call label May 27, 2020
@ruyadorno
Copy link
Contributor Author

@coreyfarrell suggested in the tooling WG that it would be nice to have a pager config (similar to git) to go with that, to which I really like 😊

@coreyfarrell
Copy link
Contributor

@ruyadorno have you considered if any standard diff CLI flags will be supported? Like pager support this is not critical but I very frequently use git diff --ignore-all-space. This is great when blocks of unconditional code gets moved into a condition:

diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js
index 6e3935d138..e6ae3952f3 100644
--- a/lib/internal/main/worker_thread.js
+++ b/lib/internal/main/worker_thread.js
@@ -113,8 +113,9 @@ port.on('message', (message) => {
     initializeDeprecations();
     initializeWASI();
     initializeCJSLoader();
-    initializeESMLoader();
+    initializeESMLoader(loaderPort);
 
+    if (!internal) {
       const CJSLoader = require('internal/modules/cjs/loader');
       assert(!CJSLoader.hasLoadedAnyUserCJSModule);
       loadPreloadModules();
@@ -122,6 +123,7 @@ port.on('message', (message) => {
       if (argv !== undefined) {
         process.argv = process.argv.concat(argv);
       }
+    }
     publicWorker.parentPort = publicPort;
     publicWorker.workerData = workerData;
 

With or without support for standard diff flags this feature will be great!

@ljharb
Copy link
Contributor

ljharb commented May 30, 2020

In general it’d be nice if it could defer to git diff - and all its options.

@ruyadorno ruyadorno mentioned this pull request Aug 26, 2020
@juliangruber
Copy link

Quickly sharing my learnings from working on https://github.com/juliangruber/npm-diff:

  • git diff plus colordiff works great, however colordiff isn't available everywhere. A JS replacement might not be hard to write
  • If less is used with colordiff, need less -R
  • In my tests it was useful to ignore any changed files with test/ and Makefile in their name, since as a consumer these don't change my situation. There could be false negatives ignoring test.
  • In my tests it was also useful to ignore line changes containing readme, _id, _from and _resolved, as these were noise

ruyadorno added a commit to ruyadorno/cli that referenced this pull request Jan 20, 2021
- As proposed in RFC: npm/rfcs#144
ruyadorno added a commit to npm/cli that referenced this pull request Jan 20, 2021
- As proposed in RFC: npm/rfcs#144
ruyadorno added a commit to npm/cli that referenced this pull request Jan 27, 2021
- As proposed in RFC: npm/rfcs#144
ruyadorno added a commit to npm/cli that referenced this pull request Jan 28, 2021
- As proposed in RFC: npm/rfcs#144
nlf pushed a commit to npm/cli that referenced this pull request Jan 28, 2021
- As proposed in RFC: npm/rfcs#144

PR-URL: #1319
Credit: @ruyadorno
Close: #1319
Reviewed-by: @isaacs
@bnb
Copy link

bnb commented Feb 12, 2021

Should this be merged?

@ruyadorno
Copy link
Contributor Author

yes! I'll just finish cleaning it up and merge it as implemented 😊

@ruyadorno
Copy link
Contributor Author

For future reference, this feature went out in npm@7.5.0 and I wrote a little overview in a blog post format over here: https://dev.to/ruyadorno/npm-diff-23dh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants