Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP docs #156

Open
leeoniya opened this issue Jul 5, 2017 · 52 comments
Open

WIP docs #156

leeoniya opened this issue Jul 5, 2017 · 52 comments
Labels

Comments

@leeoniya
Copy link
Member

leeoniya commented Jul 5, 2017

some of this stuff is briefly mentioned in https://github.com/leeoniya/domvm#templates and easily overlooked, other stuff requires reading the code of the demos, and the rest is just plain missing. i think the list below represents a fairly thorough todo list for the docs.

does anyone have a good feel for how best to prioritize the items in terms of greatest impact?

cc @lawrence-dol @iamjohnlong @sabine @tropperstyle @e1ectronic @xz64

General:

@leeoniya leeoniya added the docs label Jul 5, 2017
@iamjohnlong
Copy link

I think the patterns are key. I know the slogan is domvm wont hold your hand but having patterns defined will help newcomers, especially with the vm.state defined for people coming from react or mithril.

@leeoniya
Copy link
Member Author

leeoniya commented Jul 5, 2017

I think the patterns are key.

yeah, this and probably third-party lib interop. most will want to know whether they'll be required to re-implement all their favorite deps:

  • domvm.injectElement(el)
  • appending into from willInsert/didInsert hooks
  • {_raw: true} (innerHTML body)

drop a tip/link to theautoredraw section so users know that they don't need to clutter their code with vm.redraw() everywhere. (opt-in magic):

💡 You can set up auto-redraw using the global onevent handler. See ...

plus perhaps a router setup example with subviews. (admin backend navigation example)

I know the slogan is domvm wont hold your hand

i'd say that presenting a bunch of different patterns is in line with that statement; there's no "one right way".

@lawrence-dol
Copy link
Collaborator

I agree with the interop priority. In fact, I have a calendar widget in Mithril that I need to port to DOMVM. If I were asked to prioritize the above list, I think I would have:

Primary:

  • High level patterns https://github.com/leeoniya/domvm/wiki/View-Model-Patterns
  • High level API table to complement the dense templates rundown, à la upcoming v3 changes #147 (comment)
  • vm.config({onevent:...})
  • vm.redraw()
  • event handlers, delegation, parameters, caveat for types that cannot be bound via on* props (e.g. transitionend)
  • userspace-reserved vm.state & vm.api
  • DOM refs, namespaced refs
  • vnode {_data: ...}
  • third-party lib integration, element injection
  • third-party router integration
  • vm.emit() & vm.config({events: {...}})
  • vm.body() - what is this?
  • raw nodes/text

Secondary:

  • vm.diff()
  • vnode.patch()
  • extending domvm.ViewModel.prototype & domvm.VNode.prototype
  • immutability, (as diff optimizations)
  • return vm.node
  • _raw event tagging
  • FIXED_BODY
  • KEYED_LIST
  • LAZY_LIST
  • innerHTML
  • stream adapters/reactivity, domvm.prop()

@xz64
Copy link
Contributor

xz64 commented Jul 7, 2017

Here is my personal ordering of the topics (omitting ones that I don't know how to use or when to use)

General

  • port patterns https://github.com/leeoniya/domvm/wiki/View-Model-Patterns
  • high-level API table to complement the dense templates rundown, à la upcoming v3 changes #147 (comment)
  • vm.redraw()
  • userspace-reserved vm.state & vm.api
  • event handlers, delegation, parameters, caveat for types that cannot be bound via on* props (e.g. transitionend)
  • vm.config({onevent:...}) (guessing this includes lifecycle hooks)
  • vm.emit() & vm.config({events: {...}})
  • DOM refs, namespaced refs
  • third-party integration, element injection, couple simple examples of third-party lib interop
  • innerHTML
  • walk through a third-party router integration

@leeoniya
Copy link
Member Author

leeoniya commented Jul 7, 2017

vm.config({onevent:...}) (guessing this includes lifecycle hooks)

no, its just dom events. hooks are always called hooks. however, i do see some possible confusion introduced between emit handlers vm.config({events: {...}}) and the dom event catchall vm.config({onevent: handler})

they'll be mentioned in different doc sections, so it wont necessarily be confusing at that point but i imagine reading your own code later won't be ideal:

function View(vm) {
    vm.config({
        onevent: function(e) {
            vm.redraw();
        },
        events: {
            something: function() {}
        }
    })
}

post-1.x i personally don't use emit in my apps. what i used it for previously has been replaced by vm.root().redraw() or vm.parent().redraw(). i'm open to refining this naming confusion. onevent won't be changing since it matches both the name and function of the global domvm.config({onevent:...}).

i like to keep names short, so perhaps vm.config({emit: {...}}) or vm.config({onemit: {...}}), though with the latter i'd probably be inclined to add handling for onemit being a supplied function in addition to the current hash signature.

this also brings up the question whether it's worth adding a matching global domvm.config({onemit:...}) to be used as an event bus.

EDIT: i started doing the impl for this and decided against the single-handler signature for onemit because emit is designed to be invoked with different args, making a generic handler would mean less clarity in what args are expected for each emit event and force a switch/case pattern in the single handler, which is bad. onevent is a single handler because dom events are much more generic.

@xz64
Copy link
Contributor

xz64 commented Jul 7, 2017

ok, I missed the part where this is a list of remaining items for documentation (lifecycle hooks are already documented). thanks for the clarification

@leeoniya
Copy link
Member Author

leeoniya commented Jul 7, 2017

alright, vm.config({onemit: {...}}) now holds the emit handlers, plus an additional global domvm.config({onemit: {...}}). sorry for the BC (yet again), but i guess we've already been down that path post-3.0 w/ emit() refinement. hopefully it's settled now <- famous last words 😆

@lawrence-dol
Copy link
Collaborator

i personally don't use emit in my apps

I use emit in my largest app for synthetic events derived from input events. I don't mind a few renames when the time comes to upgrade that, which will be sooner rather than later. I think it's a useful construct to have available.

I like the use of "emit" for "emit handlers".

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Jul 7, 2017

BTW, I can't seem to get vm.config({onevent: handler}) to work, even though my global event catcher does work. Is there a trick, or do I need to dig further with a fiddle? I am using V3 as of

Nevermind; I see it was added on 2017-07-06.

And updating to 3.x-dev current, the vm onevent works fine.

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Jul 7, 2017

Also, all global actions must document the caveat of having to consider what might happen if a third-party component is used. And by that I mean a third-party DOMVM component, specifically, though global event handlers might be impacted by anything external.

@leeoniya
Copy link
Member Author

leeoniya commented Jul 7, 2017

yeah, the docs should mention that global config should only ever be setup by your own app, not by any of your components or third-party dependencies. though onemit merges rather than replaces the global handlers, so it may be okay as long as you dont have emit event naming conflicts.

@tropperstyle
Copy link

Just spent a few hours upgrading a pre-DEVMODE app to 3.1. Everything felt pretty natural, but since emit events are seeing further changes, can we consider restoring the emitted value as the first argument. Since I am already calling vm.config this doesn't seem very useful. I am likely most interested in the value I just emitted...

@leeoniya
Copy link
Member Author

leeoniya commented Jul 7, 2017

restoring the emitted value as the first argument

restoring? i don't believe it ever worked this way. the current signature for emit handlers is vm, ...args where vm is the view from which the event was emitted, which is usually not the vm where the handler is registered. there's no other good place to convey this info since ...args has to be last.

you could make the argument [and maybe you are] that one could simply emit the originating vm as one of the arguments, if needed. i'll buy that as justification for this simplification.

@tropperstyle
Copy link

tropperstyle commented Jul 7, 2017

Well actually looking at the few places I do use emit, I am using imperative composition so the registered vm and the emitting vm are the same. I didn't even realize the events bubble up the parents until looking at the source. Bubbling actually provides little use for me since the associated imperative view may not even be a child in some cases.

Regardless, In 2.x, the signature was just ...args, no prepended view. So I needed to update my handlers from:

someView.on('move', data => { ... })

to:

someView.config({
  events: {
    move(_, data) { ... }
  }
})

@leeoniya
Copy link
Member Author

leeoniya commented Jul 7, 2017

so it appears i suffer from severe short-term memory loss [1]. it looks like this was done as part of the uniformity rampage [2]. a better argument for changing this is that emit's calling convention [and bubbling behavior] is similar to that of parameterized events: onclick: [dostuff, "a", "b"] that then gets invoked as dostuff("a", "b", e, vnode, vm, data).

let's go ahead and do that then. how about ...args, vm, data?

we'd need to clarify in the docs that the vm/data is that of the emitting vm rather than the handler's vm, which could be a bit odd but would work the same way for any delegated dom event handlers in a parent vm (i think, yes it's always the vm closest to e.target [3]).

[1] eb84d6d#diff-00459a3c4659fc80d15cdb0563ce43b4
[2] #147 (comment)
[3] https://github.com/leeoniya/domvm/blob/3.x-dev/src/view/patchEvent.js#L13-L15

@leeoniya
Copy link
Member Author

leeoniya commented Jul 7, 2017

7b4561e

🙏 please forgive me, semver gods! 🙏

@lawrence-dol
Copy link
Collaborator

[Deep Booming Voice] You are forgiven my son... now go and sin no more!

@leeoniya
Copy link
Member Author

i've added a bunch of docs, can you guys please look over them and see if they are clear enough and in logical order. 🙏

@lawrence-dol
Copy link
Collaborator

I thought auto-pixel was dropped in V3?

@leeoniya
Copy link
Member Author

leeoniya commented Aug 18, 2017

no i just moved it up to micro so that nano didnt have to swallow the weight of the blacklist dict.

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Aug 18, 2017

Hmmm, I'd expect the following to be "Basic" event handlers, not "Fancy", since each is a simple function:

function rowClick(e) {}
function cellClick(e) {}

el("table", {onclick: {"tr": rowClick, "td": cellClick}}, ...);

I'd expect to need to wrap each in an array to get a Fancy event handler:

function rowClick(e, node, vm, data) {}
function cellClick(e, node, vm, data) {}

el("table", {onclick: {"tr": [rowClick], "td": [cellClick]}}, ...);

@lawrence-dol
Copy link
Collaborator

Listeners may return false as a shorthand for e.preventDefault() + e.stopPropagation().

In my experience, halting the propagation of an event nearly always causes subtle faults in any application of non-trivial size. Even if not applying default behavior, it is sometimes necessary to see an already handled event at higher levels in the DOM.

Also, does this mean specifically false or generally falsey?

@leeoniya
Copy link
Member Author

leeoniya commented Aug 18, 2017

I'd expect to need to wrap each in an array to get a Fancy event handler

i suppose this needs a more obvious explanation.

fancy listeners are all proxied by an internal handle function which does the delegation testing and arg augmentation on-demand. at the end of the day, you either have your exact function set to onclick (basic) or you have handle bound to onclick (fancy).

i could artificially not augment the arguments for non-array-wrapped delegated listeners, but the onclick binding will still be to the internal handle and will still incur the overhead associated with additional handle code.

in addition, the vm-level and global onevent callbacks are executed by handle, and those have a uniform signature that will always get e, node, vm and data, even if the trailing args array is empty.

Also, does this mean specifically false or generally falsey

well, returning nothing would be generically falsey, so yes it's explicitly false. i personally find that having a single event handler do different things and fire at multiple dom levels to be a pattern that is itself surprising. so doing stopPropagation() should not cause problems. if it does, well then just do e.preventDefault() yourself and return nothing.

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Aug 18, 2017

Just curious, do you really think that this:

function StepperView(vm, stepper) {
    var add = stepper.add.bind(stepper);

    function set(e) {
        stepper.set(e.target.value);
    }
    ...

is equally as clear as this:

function StepperView(vm, stepper) {
    function add(val) {
        stepper.add(val);
    }

    function set(e) {
        stepper.set(e.target.value);
    }
    ...

?

I only ask because this is an example, and I had to think about it for a bit to realize that the two functions were logically equivalent; it seems to me that the latter is considerably more obvious. Which may, of course, be my relative inexperience with Javascript vs. Java and C.

(Also, my mind would have followed the code just a tad easier if the SteeperView function had followed the Stepper function, since the former is used by the latter.)

@lawrence-dol
Copy link
Collaborator

Emit System

Perhaps document whether the invocation of the listener is inline or deferred to the event queue or RAF queue? That could be subtly important in some instances.

@lawrence-dol
Copy link
Collaborator

Lifecycle Hooks

Document the various hooks' arguments in this section.

@leeoniya
Copy link
Member Author

leeoniya commented Aug 18, 2017

Just curious, do you really think that this...is equally as clear as this...

no. good point.

(Also, my mind would have followed the code just a tad easier if the StepperView function had followed the Stepper function, since the former is used by the latter.)

makes sense.

Document the various hooks' arguments in this section.

yep, will do.

Perhaps document whether the invocation of the listener is inline or deferred to the event queue or RAF queue? That could be subtly important in some instances.

can you elaborate when this would be important and why "synchronous" cannot simply be assumed, considering that everything that's asynchronous is already explicitly called out?

thanks for the review!

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Aug 18, 2017

can you elaborate when this would be important and why "synchronous" cannot simply be assumed, considering that everything that's asynchronous is already explicitly called out?

Because, to me, things like "events" and "signals" are always asynchronous; while "listeners" are always synchronous. However, the emit system seems to be in a gray area where it's not clearly one or the other. It's described with nomenclature for both.

@leeoniya
Copy link
Member Author

that only answers half the question, tho. when would sync and async emit signals behave differently? why is it important?

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Aug 18, 2017

when would sync and async emit signals behave differently? why is it important?

Oh, that's easy to illustrate.

Consider, for arguments sake, an inline "redraw" function. Now consider a property get/set function that invokes an application listener when the property is to be changed, allowing the listener to alter the value or even veto on the change by returning the existing value.

Now, the application is coded to use the change listener to emit a "model-change" event while the VM uses the "model-change" event to trigger redraw of the entire view. On the surface it all seems reasonable enough for a decoupled system.

But at the point where the listener is invoked is immediately before actually setting the underlying value (since the listener can alter/veto the value). Since emitted events are also inline and redraw is also inline, the call sequence looks like:

prop("new-value") => listener => emit => emit-handler => redraw => View.render

and the view is recreated with the old value.

If the emit system is viewed as a listener system inline is intuitive; but as an eventing system the result is counter-intuitive; the app developer expected:

prop("new-value") => listener => emit 
...
emit-handler => redraw => View.render

I know, because I was that developer (using Mithril, I think). It was immensely confusing because it only actually failed when the property was set outside of an event (which in Mithril always triggered a subsequent redraw).

These details do matter, IMHO.

@leeoniya
Copy link
Member Author

leeoniya commented Aug 18, 2017

ok, that makes sense.

however, i'm not convinced that the terminology you're using has the widely-accepted expectations you're implying. i agree that "signals" can be removed. but the emit system is a bubbling event system just like dom events, which has "event listeners" and is fully synchronous - it's not a pub-sub.

however, sending events into a view externally via vm.emit(...) is a form of "signal" or "trigger". i guess we can call it "dispatch an event", as the dom does?

i do want to draw paralells between how dom events work and how the emit system works in the prose, because it doesnt introduce extra cognitive load or new terminology.

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Aug 18, 2017

the emit system is a bubbling event system just like dom events [...] we can call it "dispatch an event"

Correct me if I am wrong, but dispatching an event simply drops an event object on the event queue, it does not actually bubble the event and invoke its listeners synchronously. Therefore, to match event dispatch, the emit function should also simply drop an event on a queue. And that is a critical detail.

Technically achieving that in Javascript might be a quite different matter. :-|

@leeoniya
Copy link
Member Author

leeoniya commented Aug 18, 2017

to my knowledge, dispatching and bubbling of dom events behaves synchronously in userland, otherwise every assert() call after doClick(targ) would fail [1].

[1] https://github.com/leeoniya/domvm/blob/3.x-dev/test/src/events.js

@lawrence-dol
Copy link
Collaborator

dispatching of dom events behaves synchronously

I stand corrected, and this is even consistent with Java where dispatchEvent is synchronous and postEvent drops the event on the queue. Obviously it's been too long since I dove into the details. Therefore, I concur that the docs for emit are correct in this respect and it's sufficient to infer synchronous behavior consistent with dispatchEvent.

References to "signal" should be removed; they are, I think, misleading. And I think it should be described in similar terms to dispatchEvent, "Dispatches an event on the specified view, bubbling up through the view hierarchy, invoking event listeners in the appropriate order".

I think it would also be useful for there to be a caveat on the global event listeners, like other "global" things, describing considerations for components developed in isolation of the app.

All of this makes me wonder if providing emitAsync might not be useful, both as clarifying the difference and as a convenience.

vm.emitAsync=function("myEvent", arg1, arg2) {
    let fnc=...;
    setTimeout(fnc,0);
    }

@leeoniya
Copy link
Member Author

i updated the docs.

I think it would also be useful for there to be a caveat on the global event listeners, like other "global" things, describing considerations for components developed in isolation of the app.

"global" means the same thing in every language, with the same footguns. most devs should already understand the implications of such things.

All of this makes me wonder if providing emitAsync might not be useful, both as clarifying the difference and as a convenience.

it's extra API surface which you can add yourself, even on the ViewModel prototype if you so please. i don't see any reason to offer asynchronous code over clearer synchronous code.

@lawrence-dol
Copy link
Collaborator

That's much clearer. Small typo in "There is also a global emit listener which will fires for all emit events."

@leeoniya
Copy link
Member Author

most of the Optimization section has been written.

@leeoniya
Copy link
Member Author

leeoniya commented Aug 21, 2017

Third-Party Integration is done: https://github.com/leeoniya/domvm#third-party-integration
Arguments for lifecycle hooks have been added: https://github.com/leeoniya/domvm#lifecycle-hooks
Mentioned changelog location: https://github.com/leeoniya/domvm#changelog

getting close!

@lawrence-dol
Copy link
Collaborator

Maybe make the life-cycle hooks code so they stand out a little more:

will/didInsert(newNode) => will/didInsert(newNode)

@lawrence-dol
Copy link
Collaborator

In looking at the hooks docs, though, there's an inconsistency between will/didMount & will/didUnmount and the node's will/didInsert & will/didRemove. It seems to me that these are corresponding actions and should use the same names (Insert and Remove).

Of course, it's probably not worth changing now.

@leeoniya
Copy link
Member Author

leeoniya commented Aug 22, 2017

It seems to me that these are corresponding actions and should use the same names (Insert and Remove).

Yeah, maybe a bit.

However, i like that they're distinctively-named since they have different args and vms have corresponding vm.mount() and vm.unmount() methods that can be invoked directly, whereas vnodes do not. It would likely help with tracking down user bug reports since i dont have to ask for clarification about which willInsert hook they're talking about, especially if there are simultaneously both vm-level and node-level hooks in the same view.

@leeoniya
Copy link
Member Author

added:

  • <code> around lifecycle hooks
  • vm.body(), vnode.body, vnode.parent
  • userspace-reserved vm.state & vm.api
  • extending domvm.ViewModel.prototype & domvm.VNode.prototype

@lawrence-dol
Copy link
Collaborator

extending domvm.ViewModel.prototype & domvm.VNode.prototype

I suppose these must be part of the core? Might there be a reduction in size if these could be moved to a higher build?

@leeoniya
Copy link
Member Author

yes, these are the core. i just exposed them for easy extending.

i may move hooks, lazyList, and fancy events (+ event delegation, global onevent) from pico -> nano, but this wouldn't have any effect on user-facing builds. i may also leave them because the more stuff i move out the more conditional blocks (#ifdef) i have to have all over the core.

@lawrence-dol
Copy link
Collaborator

this wouldn't have any effect on user-facing builds

Yeah, there's no point in shuffling things out of pico unless it improves the overall design and decoupling of the core. For all practical purposes, nano is the minimum build.

@lawrence-dol
Copy link
Collaborator

After reading the docs, it's not at all clear to me what the utility for vm.body() would be. I gather it returns an array of sub-vms, but why is it useful in an app?

@leeoniya
Copy link
Member Author

leeoniya commented Aug 22, 2017

vm.parent() and vm.body() allow you to traverse the view hierarchy without having to traverse the full vtree hierarchy. they're basically convenience methods that bypass vnodes which are not also vm roots.

@lawrence-dol
Copy link
Collaborator

lawrence-dol commented Aug 22, 2017

I think the title of "What's Missing?" wrongly implies that DOMVM is deficient. I think that section should be included in the intro, under a sub-title like "What DOMVM Is Not", or something similar that denotes scope-restriction rather than functional lack.

PS: Although it's pedantic, "Into" should be "Introduction".

@lawrence-dol
Copy link
Collaborator

Also, the documentation index should, I think, be immediately after the Introduction, with the Demo Playground included in that list. It provides an overview to all the information to the page. Indeed, an argument can be made for having it as the first section and including Introduction in the list of links.

@leeoniya
Copy link
Member Author

I think the title of "What's Missing?" wrongly implies that DOMVM is deficient. I think that section should be included in the intro, under a sub-title like "What DOMVM Is Not", or something similar that denotes scope-restriction rather than functional lack.

👍

"Into" should be "Introduction"

👍

Also, the documentation index should, I think, be immediately after the Introduction

eh. domvm is meant for a technical audience. personally, i always want examples first and docs second, only if the examples already "felt right". the index is an entire extra screen of scrolling.

@lawrence-dol
Copy link
Collaborator

I just did a scan of my code and I don't use emit anymore either. Consequently I've just switched to the nano build.

@lawrence-dol
Copy link
Collaborator

@leeoniya : I suggest closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants