search

thheller / shadow-cljs

ClojureScript compilation made easy
https://github.com/thheller/shadow-cljs
Eclipse Public License 1.0
2.25k stars 178 forks source link

Extending object breaks some shadow-cljs functionality #1186

Closed cj-price closed 3 months ago

cj-price commented 3 months ago

Doing some research, I have this code:

(extend-type object
  IAssociative
  (-contains-key? [o k]
    (j/contains? o k))
  (-assoc [o k v]
    (j/assoc! o k v)))

The application works as expected, but :browser-test doesn't show any tests and hot reloading via ^:dev/after-load doesn't run the function. I am willing to investigate more if you'd like.

thheller commented 3 months ago

I'd imagine that this breaks much more than just shadow-cljs. Extending object is generally not a good idea, and especially bad for the core protocols, since many protocol checks are done to check the type of certain things. Using IAssociative that way will make cljs.core think that everything is a map.

Go nuts trying to figure out what exactly goes wrong. I'll will not waste my time on that.

cj-price commented 3 months ago

Good to know! Thanks for the reply. While I didn't see this behavior for ILookup, would you say that would also not be a good idea?

thheller commented 3 months ago

I'm unsure what you are trying to achieve in the first place. JS Objects are mutable, so extending protocols intended for persistent datastructures onto them is not great. Also JS objects can only have String keys, while CLJS maps can do pretty much anything. In general none of the core protocols make sense on the generic JS Objects, otherwise they would probably already be there.

cj-price commented 3 months ago

Well, for ILookup destructuring without the conversion of js->clj cost. For IAssociative, wanted to return a map/vector if encountering an object/array with the result of whatever function was called on it. Essentially only doing the js->clj conversion when necessary, without having to put too much thought into doing the conversion beforehand.

Granted, I am not sure any of this would yield better performance or make working with raw react any cleaner.

But from what I can tell now probably not a good idea :)

thheller commented 3 months ago

Dunno. I always advocate to use interop as much as possible and skip any conversion. Especially if the only reason is destructuring. Interop is not that bad.

Closing as this really has nothing to do with shadow-cljs.