danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.26k stars 368 forks source link

API idea: JSON Diff #175

Closed orta closed 7 years ago

orta commented 7 years ago

We care a lot about JSON files in the JS community, so pretty often I think to myself "hrm, I'd like to do a diff to know whether the package version had changes, or dependencies" - it's pretty tough to do

It's definitely a bunch of code in a Dangerfile, that so far I've been hesitant to write. Perhaps it could be a new function inside the danger.utils class?

Here's some examples of what I'd imagine this could look like:

Promise vs callback API is worth a discussion, for my vote TBH I'm a promises, mainly because I use async/await but for now I'll just write a callback

danger.utils.jsonDiff("package.json", (diff) => {
  // diff could look something like
  // {
  //   "version" : {
  //     "before": 0.0.5,
  //     "after": 0.0.6
  //   }
  // }
  if (diff && diff.version && diff.version.before < diff.version.after) {
    fail("No retreat, no surrender.")
  }
}) 

The nullability aspect is interesting, initially I thought of passing null instead of an empty object to the params, but maybe it makes more sense to just not run the closure at all if there are no changes?

orta commented 7 years ago

Could have a much more fine grained check so that things like this don't happen

screen shot 2017-03-18 at 17 08 42
orta commented 7 years ago

That could look more like

danger.utils.jsonDiff("package.json", (diff) => {
  const depChanges = diff.dependencies || diff.devDependencies
  const lockfileChanges = danger.git.modified_files.include("yarn.lock")
  if (depChanges && !lockfileChanges) {
    fail("No lock file changes - yada yadda")
  }
}) 

Or you can start picking out the specific deps that are changed and do something like http://pkgsize.com on those locally

orta commented 7 years ago

I bet @wtgtybhertgeghgtwtg would love that have that on each dep changes 💯

wtgtybhertgeghgtwtg commented 7 years ago

I hadn't realized that I had been making a name for myself. The implementation might not be that complex.

  1. Generate a JSON Patch from the original package.json to the proposed package.json (only add, replace, and remove, one entry per key).
  2. Iterate over the patch, expand the key and create an object per path. a. If op is add, before is undefined, after is value. b. If op is replace, before is the value from the original package.json, after is value. c. If op is remove, before is the value from the original package.json, after is value.
wtgtybhertgeghgtwtg commented 7 years ago

Actually, you could probably ignore op altogether.

import {createPatch} from 'rfc6902';
import jsonpointer from 'jsonpointer';

function getDiff(beforePackage, afterPackage) {
  const patch = createPatch(beforePackage, afterPackage);
  return patch.reduce((accumulator, {path, value}) => {
    const diff = {
      after: value,
      before: jsonpointer.get(beforePackage, path),
    };
    jsonpointer.set(accumulator, path, diff);
    return accumulator.
  }, Object.create(null));
}
orta commented 7 years ago

Interesting, I've never heard of these two dependencies, and there is a standard for JSON patching - perfect. Better to work on existing infra. Thanks @wtgtybhertgeghgtwtg

orta commented 7 years ago

PR landing tomorrow

orta commented 7 years ago

Shipped.