I think one reason why these issues are reported infrequently is because it is really hard to track them down. I can imagine people disable :advanced optimizations or restructure their code instead of hunting down the real cause. John went really far to isolate/demonstrate the issue.
Thanks for posting that sentinel idea, Thomas. It looks really simple. I think instead of sentinel object we could use some large random integer which could have similar performance profile as checking for boolean. I would add another possible idea: 4. we could alter google closure compiler with ClojureScript-aware renaming logic, it would preserve prefix $cljs prefix when renaming properties[1]. Also in ClojureScript compiler we would rename all cljs-related properties to use this convention. Pros: 1. This would decrease a chance of clashes. When someone runs into it - it would be quite self-describing that "$cljs" named property is involved. Cons: 1. This would lead to slightly bigger generated code (all the $cljs prefixes which would previously disappear). 2. The bigger downside IMO is that we would have to maintain our own fork of Closure Compiler. Which is not that scary with git. It is pretty easy to automate rebasing of a few patch-commits on top of arbitrary complex foreign repo. [1] https://github.com/google/closure-compiler/blob/5616a66e40c5dc6ec9be9beacfc0ea20d47bbcfc/src/com/google/javascript/jscomp/RenameProperties.java#L284 On Sunday, October 16, 2016 at 4:36:38 PM UTC+2, Thomas Heller wrote: > > FWIW I investigated the check with "true" and a sentinel value and found > them to both have a small performance impact over just checking for a > true-ish property. > > http://dev.clojure.org/jira/browse/CLJS-1658 > > The impact is really small so it might be worth the trade-off. > > /thomas > > On Sunday, October 16, 2016 at 3:59:20 PM UTC+2, David Nolen wrote: >> >> It's true that there other scenarios where you can encounter this but it >> gets reported infrequently enough that I don't think these kinds of >> property name collisions are common. >> >> Still it has come up before and I think the simplest solution least >> likely to adversely affect performance is to test for a def'onced unique >> object instead of `true`. >> >> David >> >> On Sat, Oct 15, 2016 at 8:46 PM, Antonin Hildebrand < >> antonin.h...@gmail.com> wrote: >> >>> Unfortunately, this problem is not specific to `js->clj` only. >>> >>> I believe in general under :advanced optimizations, any object which was >>> created or modified by a code which >>> was not subject of the same closure compiler optimization pass could >>> exhibit similar problems when used with ClojureScript core functions like >>> `satisfies?`. >>> >>> That also includes working with external data (your case), and working >>> with objects created/modified by adding properties by string names. >>> >>> To illustrate, I created a screenshot of cljs type instance with two >>> protocols, to see the internals in dev mode: >>> https://dl.dropboxusercontent.com/u/559047/cljs-protocol-internals-01.png >>> In the selected text I highlighted part of generated code for >>> `satisfies?` call. >>> >>> ClojureScript adds/checks $cljs prefixed properties to objects. These >>> internal properties get renamed by closure compiler. >>> So any object which happens to have one of those renamed names >>> independently added as their property will potentially confuse functions >>> like `satisfies?`. >>> >>> Possible solutions I see: >>> >>> 1. use string names for clojurescript internal properties, and avoid >>> clashes by using "unique-enough" prefix even in advanced mode - still not >>> safe solution, but would minimize clash chances >>> >>> or >>> >>> 2. start tracking which objects belong to cljs land, have one >>> "unique-enough" string name as a marker, clojurescript functions like >>> satisfies? would check this marker before proceeding further. Still dirty, >>> one could clobber cljs properties by modifying a cljs-land-object from >>> unaware Javascript code. And this would probably change behaviour of some >>> existing code. >>> >>> or >>> >>> 3. use prototypal inheritance and "hide" all ClojureScript internal >>> properties in a new link in the prototype chain, plain javascript objects >>> would miss this link so it could be easily detected, properties would not >>> clash even if they got same names. ClojureScript functions like satisfies? >>> would properly >>> walk the chain and read properties from proper link which belongs only >>> to ClojureScript. Ale we would not need any special "magic" marker - the >>> Javascript type of the link in prototype would safely identify it. >>> I believe this would be correct solution. But I guess, this would be too >>> dramatic change in ClojureScript internals and might break a lot of other >>> things I currently don't understand. Also performance could get a hit. >>> >>> Better ideas, anyone? :-) >>> >>> ps. don't use :advanced mode when programming atomic reactors in >>> ClojureScript ;-p >>> >>> On Saturday, October 15, 2016 at 10:59:14 PM UTC+2, John Szakmeister >>> wrote: >>> >>>> On Sat, Oct 15, 2016 at 2:49 PM, David Nolen <dnolen...@gmail.com> >>>> wrote: >>>> > This issue is somewhat to be expected if you're going to use >>>> `js->clj`. This >>>> > issue has nothing to do with ClojureScript compiler versions - you >>>> just got >>>> > lucky before. Google Closure will collapse properties, but some of >>>> these >>>> > collapsed properties are going to be used to determine protocol >>>> membership. >>>> > That's it. >>>> >>>> Wow. I did not that expect that at all. It makes sense, but it's >>>> unfortunate. >>>> >>>> > I suggest just avoiding `js->clj` and using your own simple helper >>>> for >>>> > recursively converting JSON into Clojure values. Changing the >>>> (admittedly >>>> > questionable) behavior of `js->clj` will only lead to more breakage. >>>> >>>> I'll definitely look at alternatives. It'd be nice if js->clj had >>>> documentation on this shortcoming though, and perhaps pointers to >>>> better alternatives. >>>> >>>> Thanks for the help David! >>>> >>>> -John >>>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Clojure" group. >>> To post to this group, send email to clo...@googlegroups.com >>> Note that posts from new members are moderated - please be patient with >>> your first post. >>> To unsubscribe from this group, send email to >>> clojure+u...@googlegroups.com >>> For more options, visit this group at >>> http://groups.google.com/group/clojure?hl=en >>> --- >>> You received this message because you are subscribed to the Google >>> Groups "Clojure" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to clojure+u...@googlegroups.com. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> -- You received this message because you are subscribed to the Google Groups "Clojure" group. To post to this group, send email to clojure@googlegroups.com Note that posts from new members are moderated - please be patient with your first post. To unsubscribe from this group, send email to clojure+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/clojure?hl=en --- You received this message because you are subscribed to the Google Groups "Clojure" group. To unsubscribe from this group and stop receiving emails from it, send an email to clojure+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
