Fork me on GitHub
#hoplon
<
2017-12-22
>
thedavidmeister02:12:42

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

thedavidmeister02:12:04

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

thedavidmeister02:12:15

commenting out hoplon.core in the ns works

thedavidmeister02:12:09

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

flyboarder02:12:38

are they even needed?

flyboarder02:12:23

I mean the attribute ones make sense

thedavidmeister02:12:27

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

thedavidmeister02:12:50

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

thedavidmeister02:12:32

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

thedavidmeister02:12:04

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

flyboarder02:12:26

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

thedavidmeister02:12:06

that could be it, would make sense

flyboarder02:12:58

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

flyboarder02:12:23

cells are for state, arguably children are not state

thedavidmeister02:12:29

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

thedavidmeister02:12:59

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

thedavidmeister02:12:13

rather than the DOM updating needing to be aware of cells

thedavidmeister02:12:28

just thinking aloud ๐Ÿ™‚

flyboarder02:12:03

a special โ€œparent awareโ€ cell perhaps

flyboarder02:12:30

or NodeList cell

thedavidmeister02:12:43

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

thedavidmeister02:12:30

NodeList cell sounds like it would have to poll, which wouldn't scale ๐Ÿ˜•

thedavidmeister02:12:48

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

flyboarder02:12:57

or maybe this can be replaced entirely with observer api?

thedavidmeister02:12:33

so that probably would bring some browser support into the picture

thedavidmeister02:12:48

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

flyboarder02:12:52

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

thedavidmeister02:12:38

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

thedavidmeister02:12:50

messing with low level prototypes

thedavidmeister02:12:22

also there are literally no comments/docs anywhere afaics

thedavidmeister02:12:12

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

thedavidmeister02:12:41

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

thedavidmeister02:12:49

i feel a bit like my hair is on fire with this one ๐Ÿ˜›

thedavidmeister06:12:52

ok, i can confirm the issue

thedavidmeister06:12:58

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

thedavidmeister06:12:10

commenting out this definitely fixes the bug in the wysiwyg

thedavidmeister06:12:36

progress ๐Ÿ™‚

flyboarder06:12:09

@thedavidmeister does it break the tests?

thedavidmeister06:12:39

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

thedavidmeister06:12:54

can chase up something more drastic later

thedavidmeister06:12:59

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

thedavidmeister06:12:07

it's only called in elem+

flyboarder06:12:08

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

flyboarder06:12:14

gonna look through commits

flyboarder06:12:03

@thedavidmeister ah itโ€™s for *-tpl macros

thedavidmeister06:12:02

well that's the other potential approach

thedavidmeister06:12:09

rather than trying to do a detection

thedavidmeister06:12:28

maybe this could be shifted into mkelem

thedavidmeister06:12:27

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

thedavidmeister06:12:47

atm native? looks like

thedavidmeister06:12:49

(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?)))

thedavidmeister06:12:07

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

flyboarder06:12:16

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

thedavidmeister06:12:35

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

thedavidmeister06:12:53

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

thedavidmeister06:12:45

so many questions, lol

flyboarder06:12:51

yep should be using satisfies?

flyboarder06:12:29

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

thedavidmeister06:12:56

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

thedavidmeister06:12:28

but not actually used

thedavidmeister06:12:00

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

thedavidmeister06:12:48

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

thedavidmeister06:12:15

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

thedavidmeister06:12:44

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

flyboarder07:12:17

yeah we are mixing extending and overriding

flyboarder07:12:08

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

thedavidmeister07:12:18

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

thedavidmeister07:12:22

well, there are 3 elements there

thedavidmeister07:12:31

the parent, the reference element and the thing being inserted

thedavidmeister07:12:58

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

thedavidmeister07:12:43

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

flyboarder07:12:09

no I think just the children is fine still

flyboarder07:12:52

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

flyboarder07:12:31

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

thedavidmeister07:12:10

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

flyboarder07:12:36

yeah I think itโ€™s warranted since this also causes your dom-cache bug

flyboarder07:12:04

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

flyboarder07:12:01

Iโ€™m hitting the pillow for tonight but ill poke a few things tomorrow

thedavidmeister07:12:23

np, i'll keep tinkering here for a bit ๐Ÿ™‚

thedavidmeister07:12:58

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

thedavidmeister07:12:31

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

flyboarder07:12:41

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

thedavidmeister07:12:33

mmm, well i saw clojure 1.9 landed

flyboarder07:12:41

yep already got that included ๐Ÿ˜‰

thedavidmeister07:12:45

so we should do some triage and plan a 7.1

thedavidmeister07:12:53

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

thedavidmeister07:12:01

there's a ton of good work in 7.1 already

flyboarder07:12:35

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

flyboarder07:12:54

7.1 proposal PR completed all the checkboxes I think

thedavidmeister07:12:55

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

thedavidmeister07:12:36

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

flyboarder07:12:53

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

thedavidmeister07:12:55

yes we've gone above and beyond that checklist

thedavidmeister07:12:23

well changing ns should definitely be its own version

thedavidmeister07:12:43

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

flyboarder07:12:13

yeah maybe save that one for Hoplon 8 hahahaha

thedavidmeister07:12:35

you could go real deep there...

thedavidmeister07:12:59

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

thedavidmeister07:12:07

jquery normalises those well

thedavidmeister07:12:33

you could end up with hoplon.jquery.events ๐Ÿ˜›

flyboarder07:12:03

oh the rabbit hole

thedavidmeister07:12:11

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

flyboarder07:12:27

anyway night ๐ŸŒ™

thedavidmeister08:12:12

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

thedavidmeister08:12:02

returning false for elements created by wysiwyg and/or contenteditable

thedavidmeister08:12:38

tbh i don't know whether these elements are created by the browser or ckeditor or both ๐Ÿ˜›

thedavidmeister08:12:16

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

thedavidmeister08:12:18

ok i think i fixed it!

thedavidmeister08:12:24

hopefully i didn't break anything else in the process

thedavidmeister08:12:47

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

thedavidmeister08:12:30

these functions are actually on the Node prototype too

thedavidmeister08:12:51

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

flyboarder19:12:23

@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

flyboarder19:12:12

I still think a refactor would be most appropriate.

alandipert21:12:13

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

flyboarder22:12:07

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

alandipert22:12:57

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

alandipert22:12:06

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

flyboarder23:12:25

@alandipert not specifically, just looking for approval other than @thedavidmeister and myself ๐Ÿ˜›

flyboarder23:12:19

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

thedavidmeister23:12:34

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

thedavidmeister23:12:54

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

thedavidmeister23:12:14

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

thedavidmeister23:12:51

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

flyboarder23:12:41

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

flyboarder23:12:53

or just roll it into 7.1?

thedavidmeister23:12:59

i started the branch off 7.1

thedavidmeister23:12:12

so probably roll it in

flyboarder23:12:23

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

thedavidmeister23:12:18

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