Clojurians Log v2

@alandipert it breaks native elements as soon as i drop hoplon.core into a ns

(ns app.main
 (:require
  [hoplon.core :as h]
  [javelin.core :as j]))

(let [el (.createElement js/document "div")
      body (.-body js/document)]
 (.appendChild body el)
 (.create js/BalloonEditor el))

this breaks

commenting out hoplon.core in the ns works

@flyboarder i think i'm going to try something like that, maybe wrapping all the prototype overrides in a native element check for safety

are they even needed?

I mean the attribute ones make sense

no idea, it seems to have some internal accounting with .-hoplonKids

if it's not needed then some low level stuff could be greatly simplified

but it also makes sense to me that there is some way to avoid what hoplon is doing if it conflicts with third party code

yep

also i wonder if we could implement IMeta for js elements and put "hoplon kids" in metadata to be more "clojurey"

yeah I was looking at that, why do we have hoplon kids? Is that just for child elements within a cell?

that could be it, would make sense

it seems like a case we should be able to negate, or remove if it breaks 3rd party stuff randomly

cells are for state, arguably children are not state

well i personally wouldn't expect low level native fns to support cells

mmm, maybe some cell-fu could be employed so the cells manage the DOM updating

rather than the DOM updating needing to be aware of cells

just thinking aloud 🙂

a special “parent aware” cell perhaps

or NodeList cell

yeah, some kind of scoped contraption keeping an eye on what should go where

NodeList cell sounds like it would have to poll, which wouldn't scale 😕

but having the parent and a cell of children in the same scope makes sense to me at a high level

or maybe this can be replaced entirely with observer api?

oooh maybe

so that probably would bring some browser support into the picture

but also seems like potentially a good way to go with modern apis

i want to implement observer api for sure, just havnt had the time

this is the part of hoplon that seems the most like black magic to me, lol

messing with low level prototypes

also there are literally no comments/docs anywhere afaics

ok, well short term i will try out a basic detection and bypass strategy

i kind of need to do something because i have a subtly broken wysiwyg in my codebase now >.<

i feel a bit like my hair is on fire with this one 😛

ok, i can confirm the issue

(set-appendChild!  (.-prototype js/Element) #(.-hoplonKids %))
(set-removeChild!  (.-prototype js/Element) #(.-hoplonKids %))
(set-insertBefore! (.-prototype js/Element) #(.-hoplonKids %))
(set-replaceChild! (.-prototype js/Element) #(.-hoplonKids %))

commenting out this definitely fixes the bug in the wysiwyg

progress 🙂

@thedavidmeister does it break the tests?

not sure yet

ideally i want to go "light touch" and keep everything as it is, except for non-hoplon elements

can chase up something more drastic later

also, what's the deal with set-setAttribute!?

it's only called in elem+

I have a feeling the hoplonKids thing is part of the experimental stuff

gonna look through commits

@thedavidmeister ah it’s for *-tpl macros

well that's the other potential approach

rather than trying to do a detection

maybe this could be shifted into mkelem

or some metadata added to the created element in mkelem that would make detection robust

atm native? looks like

(defn- native?
  "Returns true if elem is a native element. Native elements' children
  are not managed by Hoplon."
  [elem]
  (and (instance? js/Element elem)
       (-> elem .-hoplonKids nil?)))

doesn't feel super robust to me, is hoplonKids really safe to rely on? i dunno

currently it’s safe since thats how the tpl’s work, but if they change then maybe not

yeah, safe in context of the fact that native? is used relatively little

but can it reliably detect the difference between a hoplon and non-hoplon element in a more general setting?

so many questions, lol

yep should be using satisfies?

ie (satisfies? ICustomElement el) < 7.1-SNAPSHOT < (satisfies? IHoplonElement el)

mmm, there's -insert-before! in the protocol

but not actually used

looks like it's just intended for external use? but then everything in hoplon internals uses the overridden protocol

IHoplonElement is implemented for all js/Element atm, not just those created by hoplon

i actually like the js/Element extension as it means we can use IFn etc.

but that's an extension of native functionality, not an override...

yeah we are mixing extending and overriding

seems we have to support cells as children while the tpl macros return a cell

mhmm

so, let's say i want to make insertBefore have a safe fallback for native elements

well, there are 3 elements there

the parent, the reference element and the thing being inserted

do they all need to be non-hoplon, or just the parent, or something else to trigger the native functionality?

probably need to check that all three are not cells, right?

no I think just the children is fine still

kk

looking through this, I’ll need to dig deeper and see how much of it is really needed, perhaps @micha can shed some light

@thedavidmeister for now I think the native? check should be fine as a workaround

it does feel like a refactor might be possible...

yeah I think it’s warranted since this also causes your dom-cache bug

and there is a bunch of other things I am seeing which aren’t needed anymore

I’m hitting the pillow for tonight but ill poke a few things tomorrow

np, i'll keep tinkering here for a bit 🙂

also we should try to make hoplon parinfer compatible at some point >.<

i'm manually lining up parens like a schmuck over here

we still need to update tests and split for jquery/goog

mmm, well i saw clojure 1.9 landed

yep already got that included 😉

so we should do some triage and plan a 7.1

we can always push some of this stuff to a 7.2 release

there's a ton of good work in 7.1 already

true! maybe we save ourselves with a 7.1 for all the existing work and move anything else to 7.2

7.1 proposal PR completed all the checkboxes I think

don't want to fall into the neverending "one more thing" trap

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

it actually makes things worse IMO because you end up with a really spiky upgrade path when too much change at once

geez IE8

yeah, personally I would also like to move all the element constructors to their own ns (hoplon.html5) like the hoplon.svg keep core to the business logic

yes we've gone above and beyond that checklist

well changing ns should definitely be its own version

you know ppl are going to have to re-organise downstream when that happens

yeah maybe save that one for Hoplon 8 hahahaha

you could go real deep there...

like imo most of the common core functionality can be handled easily by native JS, no jquery or goog needed

except events

jquery normalises those well

you could end up with hoplon.jquery.events 😛

oh the rabbit hole

yeah that's a bike shed for another day i think

anyway night 🌙

cya

ok, so looks like native? doesn't work the way that i expected

returning false for elements created by wysiwyg and/or contenteditable

tbh i don't know whether these elements are created by the browser or ckeditor or both 😛

hmmm, might be because of text #object[Text [object Text]]

ok i think i fixed it!

hopefully i didn't break anything else in the process

need to check (instance? js/Node node) rather than (instance? js/Element elem)

these functions are actually on the Node prototype too

https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore

except setAttribute, that is only available for elements, not all nodes

@flyboarder @alandipert https://github.com/hoplon/hoplon/pull/205/files#diff-eb8092184c02e48bc9df45c8bb1a8dafR160 tests (and fixes) for hoplon checking whether an element is native? 🙂

@thedavidmeister so im glad you figured it out, however I have mixed feelings about why it’s happening, we are essentially going against the protocol by overriding the prototype. We should be using specify on the element constructor to implement those changes instead of touching the prototype of the elements

I still think a refactor would be most appropriate.

flyboarder thedavidmeister unfortunately i can't remember anything about this or why it is the way it is

@alandipert how do you feel about PR #199??

i scrolled through the diff and it looks very nice :-)

is there anything in particular you could use more detailed feedback on?

@alandipert not specifically, just looking for approval other than @thedavidmeister and myself 😛

unless you have a project you might be able to try the 7.1-SNAPSHOT on

@flyboarder yes i agree that a refactor would be better, but the prototype is already overwritten

i'm just going for a quick fix, that i can write some tests against

and then we can followup with a refactor - i'm thinking that could be one of the first things in 7.2

yep agreed!

i'm going to spend some time today just writing tests for all this

ok, ill wait to cut 7.1 until then, should we merge your fixes for this as 7.0.4?

or just roll it into 7.1?

i started the branch off 7.1

so probably roll it in

ok sounds good, ill wait till you have the tests worked out

ok, unless any there are surprises i should be able to get that sorted today