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

Prettyprinted diff mangled when using advanced compilation in cljs #51

Open
kthu opened this issue Feb 26, 2024 · 6 comments
Open

Prettyprinted diff mangled when using advanced compilation in cljs #51

kthu opened this issue Feb 26, 2024 · 6 comments

Comments

@kthu
Copy link

kthu commented Feb 26, 2024

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
Copy link
Author

kthu commented Feb 26, 2024

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.

@plexus
Copy link
Member

plexus commented Feb 28, 2024

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
Copy link
Author

kthu commented Feb 28, 2024

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

@plexus
Copy link
Member

plexus commented Feb 28, 2024

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
Copy link
Author

kthu commented Feb 28, 2024

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.

@plexus
Copy link
Member

plexus commented Feb 28, 2024

Happy to hear you found a workaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Won't fix
Development

No branches or pull requests

2 participants