lambdaisland/deep-diff2

Prettyprinted diff mangled when using advanced compilation in cljs

kthu opened this issue · 6 comments

kthu commented

Thanks for the awesome tool! We are currently using it in an internal web-based integration testing tool. It's working great right up until trying to display a pretty printed version of diffs.

It seems the pretty printer messes up the diff when compiled with advanced optimizations.

Here is a shell session demonstrating the problem:

❯ tree
.
├── deps.edn
└── src
    └── ddiff_bug
        └── core.cljs

3 directories, 2 files
❯ bat deps.edn 
   1   {:paths ["src"]
   2    :deps  { org.clojure/clojurescript {:mvn/version "1.11.132"}
   3            lambdaisland/deep-diff2    {:mvn/version "2.11.216"}}}
❯ bat src/ddiff_bug/core.cljs 
   1   (ns ddiff-bug.core
   2       (:require [lambdaisland.deep-diff2 :as ddiff]))
   3
   4   
   5   (enable-console-print!)
   6   
   7   (defn diff-view
   8     [a b]
   9     (let [diff    (ddiff/diff a b)
  10           diffstr (with-out-str (ddiff/pretty-print diff))]
  11       (println "\nDocument A")
  12       (println a)
  13       (println "\nDocument B")
  14       (println b)
  15       (println "\nDiff")
  16       (println diff)
  17       (println "\nPretty printed diff")
  18       (println diffstr)))
  19   
  20   (defn -main
  21     [& args]
  22     (diff-view {"Hi" {:foo 424128, :bar 22140, :baz 243000}}
  23                {"Ho" {:foo 424128, :bar 22140, :baz 243000}}))
  24   
  25   
  26   (-main)
  27   
❯ clj -M --main cljs.main --target node --output-to main-dev.js --optimizations none --compile ddiff-bug.core

❯ clj -M --main cljs.main --target node --output-to main-prod.js --optimizations advanced --compile ddiff-bug.core

❯ node main-dev.js

Document A
{Hi {:foo 424128, :bar 22140, :baz 243000}}

Document B
{Ho {:foo 424128, :bar 22140, :baz 243000}}

Diff
{#lambdaisland.deep-diff2.diff-impl.Deletion{:- Hi} {:foo 424128, :bar 22140, :baz 243000}, #lambdaisland.deep-diff2.diff-impl.Insertion{:+ Ho} {:foo 424128, :bar 22140, :baz 243000}}

Pretty printed diff
{+"Ho" {:bar 22140, :baz 243000, :foo 424128}, -"Hi" {:bar 22140, :baz 243000, :foo 424128}}

❯ node main-prod.js 

Document A
{Hi {:foo 424128, :bar 22140, :baz 243000}}

Document B
{Ho {:foo 424128, :bar 22140, :baz 243000}}

Diff
{#lambdaisland.deep-diff2.diff-impl.Deletion{:- Hi} {:foo 424128, :bar 22140, :baz 243000}, #lambdaisland.deep-diff2.diff-impl.Insertion{:+ Ho} {:foo 424128, :bar 22140, :baz 243000}}

Pretty printed diff
{#Xo {:+ "Ho"} {:bar 22140, :baz 243000, :foo 424128},
 #Wo {:- "Hi"} {:bar 22140, :baz 243000, :foo 424128}}

It seems minimized names (as seen in the Xo and Wo tags here) are confusing the printer. The two tags seen here are minimized versions of $lambdaisland$deep_diff2$diff_impl$Insertion$$ and $lambdaisland$deep_diff2$diff_impl$Deletion$$

kthu commented

I played around a bit and found that if I add the following two entries to lambdaisland.deep-diff2.printer-impl/print-handlers and set :pseudo-names to true when building the example project above, I can no longer make it break.

'$cljs$core$PersistentArrayMap$$
map-handler

'$cljs$core$MapEntry$$
map-entry-handler

But of course depending on :pseudo-names being true is less than ideal.

Making the names work in code with symbol renaming might need some significant rearchitecting. Deep-diff is typically used for dev tooling, what is your use case for needing it in a production build?

If someone comes up with an approach that works for clj and cljs and doesn't introduce breaking changes then a PR would be much appreciated, but I don't currently see this as a priority.

kthu commented

Our use case is a integration testing tool we use internally. We use deep-diff2 to visualize the differences when the results are not as expected.

I was able to make it work, but the approach feels kind of dirty: main...kthu:deep-diff2:main

If you are interested I can make a PR

Hmmm ok if that works then there's probably a better solution that's not too hard to do. I'll try to have a look but it might be a few weeks before I get to it.

kthu commented

Great. Thanks!

It turned out the records from diff_impl had to be instantiated as well. I don't quite understand why that was not needed in the simple data I was initially testing with. I pushed an update to my fork.

We are now running my fork in our app and have not been able to break it so far. I'll comment here if we stumble upon a diff that still creates problems.

Happy to hear you found a workaround!