Clojurians Log v2

@thedavidmeister fixed!

@flyboarder hmm, that last point might cause me issues

i'll see if i can do what i need juggling some wrappers

yah Implementing IFn is a Hoplon feature and it's usage on native elements will upgrade them. causes my tests to fail

if using an element with hoplon overrides the protocol unconditionally then that makes hoplon unfriendly to 3rd party libs

actually nm

because you removed the protocol override at the same time it might not matter

hang on, i'll try some things out...

yes ok cool, the issues i used to have are gone in 7.2 so i can use hoplon elements with ckeditor5 now

@thedavidmeister woot!

it's pretty cool actually, i have a fn that takes a cell and an el and then builds an inline wysiwyg into the element that syncs with the cell

slick!

before it was a mix of native elements and hoplon stuff to avoid the protocol overrides, now it's all hoplon

awesome! the refactor was pretty straight forward, basically it wraps hoplon elements in ->hoplon to upgrade them

yeah i saw that

any idea what the overhead of calling specify! per-element vs. extending js/Element globally is?

the creation of the element might be slightly longer because specify! mutates the object, where the overrides did it once, however size of object should be the same

but i think it’s still better than using specify and cloning the object

well if we only support cell children management through public hoplon fns

can we just extend js/Element?

why do we need to do it per-element?

if we dont do it per hoplon element the elements arnt upgraded and ready for template macros

ok i think i need to read what you did more closely

there is no way for a native element to be “aware” of cell children unless it’s upgraded, doing so will give DOM Exception 8 (not an element)

so to have hoplon elements be “ready” for cells they need to be upgraded a head of time

to make tests pass for native parents, we pass them through the public API instead of using the protocol methods

thats probably why we extended the prototypes in the first place, to make all elements support cell children, but that implicit behaviour breaks 3rd party libs

yes, it's also probably much slower than native fns

i guess i'm asking why does anything need to be "aware" or "ready" of anything?

just use .appendChild() if you want native behaviour and append-child! if you want hoplon managed behaviour

thats correct

so why do we need to "upgrade" anything?

to support cells as childred in the form (div :someattr someval (*-tpl))

otherwise you will get not element exception

why not implement IHoplonElement for js/Element then use append-child! to implement everything on top of that?

well by implementing on js/Element, we are extending every element, not just hoplon elements, the intention was to avoid doing anything to native elements, but if it’s faster to modify them all we can do that too

extending is fine IIRC

it was overriding that caused issues

no third party lib is going to be expecting append-child! to exist or work a specific way, unless it is hoplon aware anyway

mhm we still extend, but on element creation instead of globally

but obviously everything wants to use the protocol and expects it to work exactly as the spec

what i'm saying, is that why even have the idea of "a hoplon element"?

why not just extend all elements once, globally

we could do that, however the problem is still the same, how do you implement cells as children, either a hoplon element that can handle the children or a “JavelinCellElement” which would be an element implementing the cell protocol

you have to be “aware” on at least one side of the equation

we can extend globally but then we need to track separately which elements implement the hoplon features, back to property checking

cells as children doesn't even totally work right now

https://github.com/hoplon/hoplon/issues/207

currently we are expecting the cell to be dereferenced before passed to those, as per the code, otherwise we need to check for an deref the cells as they come in

right, so the element isn't really handling cells as children

it's more like, there's an adapter that watches cells and manipulates the element with the cell's value (which is dom elements)

right but that is tied to the element as a method

does it have to be? could things be juggled so that el and children are args to a fn with a scope, rather than having a property on the el itself?

(defn with-children
 [el children]
 (j/with-let [el el]
  (do-watch children
   (fn [o n]
    ... diff o and n and update el ...))))

sort of like this

I see what you are saying, that could probably work, we could move that atom to a let instead around the element, however that could cause garbage collection issues when the element is destroyed

mmm is it possible to implement IWatchable for dom elements?

I dont see why not

could we do some cleanup that would help gc if we saw the element be destroyed?

not sure, I would say this relates to https://github.com/hoplon/hoplon/issues/127

reading...

on a sidenote though, even if we keep the data on the element it might make sense to implement it as metadata

that seems to be how cljs native does stuff like specify!, memoize, etc.

yeah this nesting question is unsolved currently though

well either way, i'm not seeing any perf issues just clicking around a bit

i'm probably just prematurely optimising 😛

7.1: {:id :app, :n-calls 5, :min "6ms", :max "50ms", :mad "13.76ms", :mean "15.6ms", :time% 100, :time "78ms"} 7.2: {:id :app, :n-calls 5, :min "9ms", :max "40ms", :mad "9.12ms", :mean "17.2ms", :time% 100, :time "86ms"}

@thedavidmeister ^

render an app 5 times

it is about 10% slower

not disastrous but not ideal either

I think thats acceptable for a new internal implementation, we can improve for performance moving forward

considering it doesn’t break anything or require changes

i think the pros outweigh the cons yes

overriding the protocol is a real pandora's box for 3rd party compatibility

ah also, i left some comments on a review

java.lang.ClassCastException: java.lang.Boolean cannot be cast to clojure.lang.IFn

i'm seeing this on staging builds

i reckon it's this https://github.com/hoplon/hoplon/pull/216/files#diff-a70727c8efb8b692aa114a42655b1f7bR127

you've accidentally shadowed bust-cache

so tests pass but builds fail

@flyboarder if you fix ^^ can you push a new snapshot to clojars so i can re-build?

@thedavidmeister pushing…

I found that removing cells doesnt work because we lose the reference key for the watcher, if we store this in meta data on the cell we could pull it out again

i have done similar things using meta on cells, e.g.

(defn debounce-cell?
 [c]
 (and
  (j/cell? c)
  (::debounce-cell-ms (meta c))))

(defn debounce-cell
 "debounce a non-debounce cell"
 [c ms]
 {:pre [(j/cell? c)
        ; debouncing an already-debounced cell is almost certainly a mistake
        (not (debounce-cell? c))
        (number? ms)]
  :post [(debounce-cell? %)]}
 (j/with-let [r (if env.data/testing?
                 ; don't want to deal with debounce making almost everything async on CI.
                 (j/cell= c)
                 (let [t (j/cell nil)]
                  (j/with-let [d (j/cell @c)]
                   (add-watch c (gensym)
                    (fn [_ _ _ n]
                     (.clearTimeout js/window @t)
                     (reset! t
                      (h/with-timeout ms
                       (reset! d n))))))))]
  (alter-meta! r assoc ::debounce-cell-ms ms)))

(defn safe-debounce-cell
 "debounce a cell, or return an already debounced cell if ms is compatible"
 [c ms]
 {:post [(debounce-cell? %)]}
 (cond
  (not (debounce-cell? c))
  (debounce-cell c ms)

  (= ms (::debounce-cell-ms (meta c)))
  c

  :else
  (throw (js/Error. "Attempted to debounce a debounce cell with incompatible ms."))))

this might be a 7.3 thing though?

7.2 is already getting biggish

yeah, I think 7.2 is largely complete, need to do house keeping on the related tickets

https://github.com/hoplon/hoplon/milestone/3

clojure.lang.ArityException: Wrong number of args (2) passed to: impl/prerender/->frms--6522

^^ from new build

@flyboarder https://github.com/hoplon/hoplon/pull/216/files#diff-f0fa6154ec052e13d11739e16034d8f5R25 suss

(->frms in-path slurp) should be (->frms (slurp in-path)) i think

where?

https://github.com/hoplon/hoplon/pull/216/files#diff-f0fa6154ec052e13d11739e16034d8f5R25

i'll put a comment up

lol, <h1 "Embedded Hoplon example"/> whoever wrote this is using hoplon too much

#178 is fixed by 7.2.0-SNAPSHOT! 🎉

@mudphone can you check 7.1.0 stable release too?

doing that now … 🙂

It works on my machine ™️ . 🎉

great

must have been some issue in one of the snapshots

actually we did fix some of the prototype overrides in 7.1, could have easily been that

then @flyboarder took it to the next level in 7.2

great, thanks!

I’m guessing the “prototype overrides” fix might be a bit over my head, without a lot of explaining?

not a lot of explaining really

hoplon was just overriding some low level global functions that other libs expect to work in a certain way

which obviously works... until it doesn't 😉

in 7.1 we changed it to only override those fns if the element was created by hoplon

in 7.2 there are no more overrides

ah cool

I owe you guys one. You can collect at the next Hoplon conference in Hawaii.

haha yup

well it was breaking my wysiwyg editor too 😛

Ha ha, cool. Well, it sounds real clean now.

working on it...

had to give up a little "magic" but in theory hoplon plays much nicer with libs now 🙂

@flyboarder do you know what this is about?

(when-not (:static attrs)
    (merge-kids elem nil nil)
    (add-children! elem kids)))))

or @micha @alandipert ^^ in mksingleton

why do we need mksingleton at all?

(defn head
 [& args]
 (apply (.-head js/document) args))

wouldn't this work using standard IFn logic? ^^

I moved a few issues to 7.3 milestone as they would require more refactoring.