Fork me on GitHub
#hoplon
<
2018-01-02
>
thedavidmeister02:01:35

@flyboarder hmm, that last point might cause me issues

thedavidmeister02:01:43

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

thedavidmeister02:01:39

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

thedavidmeister02:01:23

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

thedavidmeister02:01:10

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

thedavidmeister02:01:16

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

thedavidmeister02:01:28

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

thedavidmeister02:01:14

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

thedavidmeister02:01:00

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

flyboarder02:01:53

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

thedavidmeister02:01:22

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

flyboarder02:01:10

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

flyboarder02:01:53

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

thedavidmeister02:01:32

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

thedavidmeister02:01:40

can we just extend js/Element?

thedavidmeister02:01:46

why do we need to do it per-element?

flyboarder02:01:02

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

thedavidmeister02:01:41

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

flyboarder02:01:47

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)

flyboarder02:01:47

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

flyboarder02:01:37

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

flyboarder03:01:09

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

thedavidmeister03:01:20

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

thedavidmeister03:01:56

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

thedavidmeister03:01:18

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

flyboarder03:01:44

thats correct

thedavidmeister03:01:54

so why do we need to "upgrade" anything?

flyboarder03:01:34

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

flyboarder03:01:58

otherwise you will get not element exception

thedavidmeister03:01:08

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

flyboarder03:01:14

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

thedavidmeister03:01:40

extending is fine IIRC

thedavidmeister03:01:46

it was overriding that caused issues

thedavidmeister03:01:43

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

flyboarder03:01:46

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

thedavidmeister03:01:21

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

thedavidmeister03:01:00

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

thedavidmeister03:01:27

why not just extend all elements once, globally

flyboarder03:01:42

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

flyboarder03:01:17

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

flyboarder03:01:22

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

thedavidmeister03:01:26

cells as children doesn't even totally work right now

flyboarder03:01:50

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

thedavidmeister03:01:58

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

thedavidmeister03:01:43

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

flyboarder03:01:08

right but that is tied to the element as a method

thedavidmeister03:01:07

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?

thedavidmeister03:01:27

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

thedavidmeister03:01:36

sort of like this

flyboarder03:01:37

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

thedavidmeister03:01:39

mmm is it possible to implement IWatchable for dom elements?

flyboarder03:01:01

I dont see why not

thedavidmeister03:01:47

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

thedavidmeister03:01:14

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

thedavidmeister04:01:05

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

thedavidmeister04:01:36

yeah this nesting question is unsolved currently though

thedavidmeister04:01:02

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

thedavidmeister04:01:11

i'm probably just prematurely optimising 😛

flyboarder04:01:17

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"}

flyboarder04:01:29

render an app 5 times

thedavidmeister04:01:42

it is about 10% slower

thedavidmeister04:01:05

not disastrous but not ideal either

flyboarder04:01:36

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

flyboarder04:01:54

considering it doesn’t break anything or require changes

thedavidmeister04:01:54

i think the pros outweigh the cons yes

thedavidmeister04:01:19

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

thedavidmeister04:01:16

ah also, i left some comments on a review

thedavidmeister04:01:23

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

thedavidmeister04:01:29

i'm seeing this on staging builds

thedavidmeister04:01:48

you've accidentally shadowed bust-cache

thedavidmeister04:01:36

so tests pass but builds fail

thedavidmeister04:01:33

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

flyboarder05:01:58

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

thedavidmeister06:01:30

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

thedavidmeister06:01:33

(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."))))

thedavidmeister06:01:44

this might be a 7.3 thing though?

thedavidmeister06:01:59

7.2 is already getting biggish

flyboarder06:01:50

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

thedavidmeister07:01:53

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

thedavidmeister07:01:58

^^ from new build

thedavidmeister07:01:02

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

thedavidmeister07:01:45

i'll put a comment up

thedavidmeister07:01:09

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

mudphone07:01:42

#178 is fixed by 7.2.0-SNAPSHOT! 🎉

thedavidmeister07:01:31

@mudphone can you check 7.1.0 stable release too?

mudphone07:01:41

doing that now … 🙂

mudphone07:01:25

It works on my machine ™️ . 🎉

thedavidmeister07:01:24

must have been some issue in one of the snapshots

thedavidmeister07:01:02

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

thedavidmeister07:01:31

then @flyboarder took it to the next level in 7.2

mudphone08:01:50

great, thanks!

mudphone08:01:15

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

thedavidmeister08:01:42

not a lot of explaining really

thedavidmeister08:01:02

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

thedavidmeister08:01:14

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

thedavidmeister08:01:51

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

thedavidmeister08:01:57

in 7.2 there are no more overrides

mudphone08:01:30

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

thedavidmeister08:01:48

well it was breaking my wysiwyg editor too 😛

mudphone08:01:13

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

thedavidmeister08:01:57

working on it...

thedavidmeister08:01:54

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

thedavidmeister08:01:17

@flyboarder do you know what this is about?

thedavidmeister08:01:21

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

thedavidmeister09:01:42

why do we need mksingleton at all?

thedavidmeister09:01:10

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

thedavidmeister09:01:42

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

flyboarder20:01:14

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