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

upcoming v3 changes #147

Closed
leeoniya opened this issue Jun 1, 2017 · 66 comments
Closed

upcoming v3 changes #147

leeoniya opened this issue Jun 1, 2017 · 66 comments
Labels

Comments

@leeoniya
Copy link
Member

leeoniya commented Jun 1, 2017

  • views will no longer be auto-keyed by the passed-in model. it's too surprising and i've had to explain it too many times. explicitly keying by the model when needed isnt all that difficult.
  • domvm.lazyList() creator & domvm.LAZY_LIST flag for creating deferred homogenous children that can reuse old vnodes without additional allocation.
  • render() will be able to return vm.node (the old vnode) to prevent/optimize redraw when no changes.
  • xlink:href support in svg
  • vm.diff() & vm.hook() will be removed in favor of what is already possible:
    • direct assignment vm.hooks = {}
    • new: vm.config({hooks:..., diff...})
    • object return {hooks: ..., diff:..., render: ...}
    • externally passing in through opts: {hooks: ..., diff: ...}
  • dist builds may be split into a separate branch, since merging and rebasing are currently a nightmare and always result in conflicts.

v3 should be ready within the next couple weeks.

@e1ectronic
Copy link

Should I implement vm.diff(...) behavior inside render() to be compatible with the upcoming release?

@leeoniya
Copy link
Member Author

leeoniya commented Jun 1, 2017

in v3 you can just replace vm.diff(...) with vm.diff = . right now this wont work though. the object-return style will continue to work the same as before: return {diff:..., hooks:..., render:...}

in v3 you could implement your own diff inside render, by storing prior state within the view closure, diffing on each redraw and returning either a fresh vtree template or the current vm.node to indicate no changes.

@e1ectronic
Copy link

That's awesome! Thanks

@leeoniya
Copy link
Member Author

leeoniya commented Jun 1, 2017

to be really future-proof, i recommend using the object-return pattern (or implement the diff yourself in render once vm.node return is implemented). i doubt that direct assignment will be promoted or even documented (for hooks, too). once this stuff is refactored and refined, i'll have a better grasp on the ergonomics. in general, direct assignment makes for a crappy, non-discoverable api.

none of the stuff mentioned here is set in stone, yet. hold off on making any changes just yet.

@l-cornelius-dol
Copy link
Collaborator

in general, direct assignment makes for a crappy, non-discoverable api.

Yes, I agree. I do not like a direct assignment API. It also prevents validation, read-only values, and in general being unable to transparently introduce many changes without breaking backward-compatibility. That is, it's too brittle.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 1, 2017

maybe the replacement would be to provide a single setter, to prevent the vm from continuing to grow public hair:

vm.config({
  hooks: {},
  diff: function() {},
});

then internally i can also run vm.config(opts) to keep things DRY

@l-cornelius-dol
Copy link
Collaborator

What are the goals of V3?

@leeoniya
Copy link
Member Author

leeoniya commented Jun 1, 2017

"views will no longer be auto-keyed by the passed-in model" is the main goal. this has surprised many people and it's clear that this is an unneeded point of friction. in general, api refinement is always a goal when BC is involved (which mandates a major semver bump).

the overview at the top is the answer to your question. the reason it's v3 and not v v2.2 is because of semver/BC.

@l-cornelius-dol
Copy link
Collaborator

Good to know; I am a fan of API refinement, and I dislike when there are multiple ways of doing the same thing... unless there are defensible use-cases that warrant it. I would support removing any duplication of functionality from the API. If there are 3 ways to set hooks, let's just pick one.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 1, 2017

it's difficult to provide a single way that satisfies multiple styles.

  • i like having a concise definition of most views where they simply return a bare render. to take that view and add a diff optimization to it, all that must be added is vm.config() rather than switching to an object-return style.
  • some people will prefer the fully explicit object-return style everywhere.
  • others prefer es6 class views
  • externally, vm.config() allows you to add hooks to pre-initialized vms, as was possible with vm.hook().
  • in declarative subviews, the only way to pass down these things outside of model is via opts. hooking those up automatically prevents the subview from having to know to internally invoke vm.config() or re-return them.

while i agree that having "one way" to do things would be ideal, there really are multiple scenarios where it cannot always work. in theory, vm.config(opts) that's automatically executed during init on any passed-in opts would work for all cases. this eliminates the need for having an object-return signature at all, so 2 ways instead of 3. but the object-return sig also provides a path for further uniformity in view defs and a clear indication of what's being returned.

@l-cornelius-dol
Copy link
Collaborator

All good points. Sounds like it fits into "unless there are defensible use-cases that warrant it".

@l-cornelius-dol
Copy link
Collaborator

l-cornelius-dol commented Jun 1, 2017

And that's one of the great strengths, and weaknesses, of JavaScript, that there's no one "best" way to do things is almost ubiquitously true of everything.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 3, 2017

if anyone is interested in testing, all the public-facing changes listed above besides xlink:href support are now live in the 3.x-dev branch, which i've switched be default.

render(vm, model, key, opts, oldVals, newVals) now carries the 2 additional arguments that provide the previous and current return values from any present diff(), you can already assume they are somehow different because render() was not prevented. this gives you the ability to do shallow patching of vm.node and return it to prevent a full re-render. prior to this, diff had a 2-stage signature: {vals:..., then:...} to facilitate this. you can see the changeover here: 40ab7c0?diff=split.

i think the new design is more powerful, logical (it keeps the partial templates within render()) and easier to explain. it also simplifies the internals a lot.

docs still need to catch up (hooks, diff), but i've removed the autokeyed-by-model section from them (DOM Recycling).

@leeoniya
Copy link
Member Author

leeoniya commented Jun 3, 2017

i actually have some serious concerns about removing the model auto-keying. it may not be such a good fit for domvm's api after all. i'd like some feedback to see if this change causes more problems than it solves.

the core issue is that we have a closure that accepts some model during init. in v2, that model was more-or less guaranteed to be persistent within this view due to auto-keying, unless you explicitly opted out of this and then you simply ignore the model in the closure and use the always-current vm.model or the model passed to render(vm, model).

in v3, this guarantee would be weakened significantly. now you have other problems like unintentional DOM recycling, view state transfer and a stale closured model. it seems there are more caveats to the new situation than the old. we'd have to recommend to ignore the closured model when the view is unkeyed and only ever use vm.model outside of render().

we can change the API to remove model from being passed into either the closure or render() and simply rely on using vm.model. this would be similar to React's always-current this.state. i'm not particularly thrilled with this prospect, since libs primarily differentiate by their APIs, each with their own trade-offs. this would also cause significant BC.

@tropperstyle
Copy link

FWIW, I grew tired of constantly checking and updating my view vs render arguments. I now avoid the closured model all together in favor of vm.model and leaving out the render arguments entirely. Even having to explain to a newcomer why the signature is the same and beware of the gotcha of a stale reference seems like a burden of unnecessary cognitive load.

Does render(vm, model, key, opts, oldVals, newVals) really need to be 6 arguments? Why not remove the first 4? Aren't model key and opts all available from the vm passed to the view?

@leeoniya
Copy link
Member Author

leeoniya commented Jun 3, 2017

here are a few considerations handled by the current design.

a passed model argument minifies to a single character. while vm.model can only be mangled down to 7. this difference is significant when multiplied out in a large app. same applies to the other args. also, a model arg can be renamed to state by the end user, whereas vm.model cannot. the vm has a lot of implementation details that i would rather not publically promote/commit to. vm.model is currently an implementation detail. a function signature commitment allows for more flexibility to iterate domvm's internals in the future.

when you realize that render is recreated for each view, for stateless but model-bound views it may make sense to share render across instances while still having access to the vm and args (thus extracting it from the closure). this is something OO and class-based inheritance offer for free and it does matter [1]. the tldr is that render is currently fully extractable/sharable across view instances. such an optimization, though, is best handled by using ES6 class views.

given how trivial it is to ignore and omit everything besides the vm arg in both the closure and render, i dont see a very compelling case for reducing the arg count. i agree that it's not optimal to have the newVals and oldVals at the end of the chain, but they are the 0.1% case when you insist on doing your own diffing (that cannot be handled by diff) or super-optimization.

as for the stale model, it's a side-effect of being able to operate in isolation on a non-global, mutable state. in react, the parent must always pass down data (or it must be pulled from some global via mobx/redux). in the former case, refactoring a component involves updating all ancestors to pass the data down a different path (eww). in the latter, magical globals or some shared echo-chamber event bus.

if we decide that an extractable render is a far-fetched case, then its sig can be modified to render(model, oldVals, newVals), but i think ignorable arg reduction is an orthogonal topic.

sorry for the brain dump.

[1] https://www.reddit.com/r/javascript/comments/6enbvb/classes_vs_closures_performance/

@leeoniya
Copy link
Member Author

leeoniya commented Jun 3, 2017

model can be made into a getter that returns vm.model: model().moo. this way we balance minification (3 chars) and keep the impl private. but it's fairly ugly.

the ideal solution here would be for model to be a proxy object (that's always current), but the perf would almost certainly drop and support for IE would be impossible.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

@leeoniya
Copy link
Member Author

leeoniya commented Jun 3, 2017

another thing you can do to keep the closured model fresh is adding a one-liner to render() that minifies to 3 chars and has basically 0 overhead: closuredModel = model.

or a slightly more expensive willUpdate vm hook that does the same thing but stays out of render().

@l-cornelius-dol
Copy link
Collaborator

l-cornelius-dol commented Jun 3, 2017 via email

@leeoniya
Copy link
Member Author

leeoniya commented Jun 4, 2017

Would it make sense for the model to be an object created by DOMVM with the current arg value beinga property of that object?

the vm is already this object

@leeoniya
Copy link
Member Author

leeoniya commented Jun 4, 2017

how would you guys feel about adding another view type. it would be easy to do and caveat-free.

var v2 = domvm.defineView2;

v2(View2, state);

function View2(vm) {
  return function(vm, oldDiff, newDiff) {
    return el("div");
  };
}

this view would not be auto-keyed and would not inject the initial model or anything besides the vm into the closure. you would operate only on vm.* props directly. a matching domvm.createView2 would also be needed.

the hardest part would be meaningfully naming each version.

if you understand the caveats of the original views and are not bothered by the extra args, then the original views can cover all cases as they do today.

@l-cornelius-dol
Copy link
Collaborator

l-cornelius-dol commented Jun 5, 2017

Now that I am at work, I have the time & energy to more fully consider this thread.

Caveat: I am looking at this from the app developer's perspective, looking for a tight delineation of what "belongs" to DOMVM and what "belongs" to me. I don't like having to screw with properties of vm; in my opinion, the only thing that vm should provide is a public API of functions to interact with DOMVM -- it should not be the place where I store stuff for my app.

when you realize that render is recreated for each view, for stateless but model-bound views it may make sense to share render across instances while still having access to the vm and args (thus extracting it from the closure).

I can think of important use-cases for having a non-closured, shared render function with the model/state injected in. Retaining that is very important.

given how trivial it is to ignore and omit everything besides the vm arg in both the closure and render, i dont see a very compelling case for reducing the arg count.

This is the kind of situation where I prefer to have an "opt in", with such values provided in a map/object. However, I agree that the ability to simply omit trailing arguments makes the case for reducing arguments weaker. With that said, I have found over the years that adding arguments is very much not future-proof and usually indicates that the function is too broad, or that the API has been too ad hoc over time.

model can be made into a getter that returns vm.model: model().moo

Ugh. Actually, that was my initial reaction, but after chewing through the rest of the thread and attempting to come up with alternatives, this was one of the conclusions I reached at the end.

another thing you can do to keep the closured model fresh is adding a one-liner to render()

I've resorted to that trick somewhere, in a Mithril app, I think. I wasn't thrilled with it, but it got the job done. I'd rather the problem didn't exist.

Would it make sense for the model to be an object created by DOMVM with the current arg value being a property of that object?
...
the vm is already this object

It is from the point-of-view of DOMVM, but less so from the point-of-view of the app. The minute I have to store something in vm or get something that belongs in my app domain out of it, I feel the API is already broken. I would like to see something akin to:

function View(vm,state) {
    // state === same as render's state always
    // state.model === app model; name determined by DOMVM :-(
    return function render(vm,state) {
        // state.model always fresh whether access via closure or argument
    }
}

With render then still extractable:

function View(vm,state) {
    // state === same as render's state always
    // state.model === app model; name determined by DOMVM :-(
    return render;
}

function render(vm,state) {
    // state.model always fresh whether access via closure or argument
}

how would you guys feel about adding another view type. ... view2.

Maaaaybeee. But the simpler option should be view and the more complex view2. However, my gut tells me this is not the answer. One thing I particularly don't like is DOMVM's dual use of the word "model". It seems it's ambiguously used as the application model and the view's model. But vm is the view model, and the currently named vm.model is really the application's view state within the view-model. Some of that state may be the app's model, but what it is, and what it's named should be in the app domain (notwithstanding practical realities). Perhaps it's the method of injecting current state that's broken? Maybe it should be done thus:

function View(vm,state) {
    return {
        modelName: "data", // optional
        render: function render(vm,state) {
            // state[modelName] set to xxx by DOMVM from `view(View,xxx,...)`
            // vm.oldDiff and vm.newDiff could be there for the "0.1%".
            // as are `vm.key` and `vm.opts`?
        }
    };
}

With the above said, it seems like a big part of the confusion is that the model data is accessible via two arguments because the render function is enclosed in the view init function. This could be mitigated by encouraging a form that always has the render function as a separate function at the same level as the View function. It could also be mitigated with an init function returned as one member of an object, of which the render function is another member. I would support changing View functions to only return an object, { init: ..., render: ..., hooks: ... model: ... } and just remove the ambiguity.

Lastly, I mentally distinguish between things that DOMVM defines and supplies in vm (a public API, key, opts) from things my app defines and supplies (render, initialization code, hooks, and model).

Maybe "model" should be a vm function which returns the current model supplied in the argument, instead of an argument passed in; then it can't be closed-over. In this case, I'd still want to have a state object available to render to store it's specific local state of the view that isn't part of the data-model driving the view, whether that was supplied as an argument or via vm.state.

Also, I think that the "key" is not something that should be of interest to the parent view; it should be something that the sub-view sets using which determines whether it represents logically the "same" node or a "different" node. It would usually be derived from the model. With that said I can think of situations where the key would be determined by the parent. So perhaps that means that the key is actually two optional parts, one from the parent joined with one from the child?

@l-cornelius-dol
Copy link
Collaborator

In re-reading my comment, another important point occurs to me. I said:

But vm is the view model, and the currently named vm.model is really the application's view state within the view-model.

But that's not the entire picture. There's:

  1. DOMVM's view-state, which should be entirely hands-off to the app.
  2. The View's view-state, which should be entirely hands-off to DOMVM.
  3. Injected view-state (the model), which should be entirely hands-off to DOMVM.

I feel like DOMVM conflates 1 & 2, while leaving the app developer to (instinctively) conflate 2 & 3 to avoid polluting 1 with stuff for 2.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 6, 2017

thanks for the thorough feedback. i don't have too much time to respond to each point but i've read through everything and want to refine the api.

  • the view closure and the vm are the carriers of private and externally accessible state/api, respectively. i dont see a benefit of initializing and tracking a third state wrapper for the sake of artificially splitting user concerns and domvm concerns since there is no clear boundary between them. the vm is the only way to expose internal view state or api to external consumers (which the user would have to touch anyhow).
  • model is not a great name and i agree it's confusing since it's not the view model. i would like to rename it either vm.data (to match the terminology for user-defined vnode.data) or vm.state.
  • let's move the model-staleness concerns to manifest only from explicit aliasing in userspace rather than the lib's closure injection (see bottom of this comment)
  • expand the View declaration api to more flexible patterns so extractable render is possible by default.
// plain-object view
const View = {
    init: function(vm) {},
    hooks: {},
    diff: function(vm) {},
    render: function(vm, oldDiff, newDiff) {},
};

// object-returning closure
function View(vm) {
    return {
        init: function(vm) {},        // redundant w/closure invocation?
        hooks: {},
        diff: function(vm) {},
        render: function(vm, oldDiff, newDiff) {},
    };
}

// render-returning closure (shorthand for object-returning closure)
function View(vm) {
    return function(vm, oldDiff, newDiff) {
        
    };
}

// functional view with state
// todo: need a way to discern this from non-functional view
// todo: where do oldDiff, newDiff go?
function View(vm, arg1, arg2) {
    return el("div");
}

class MyView extends View {
    init() {}                        // redundant w/constructor?
    diff() {}
    render(oldDiff, newDiff) {}
}

by only ever injecting vm, we should have a much cleaner api. for my own purposes, i can replicate the current closured-model behavior in userspace and worry about the model becoming stale myself, while still gaining most of the benefits of minification, less the aliasing.

function View(vm) {
    var model = vm.data;

    return function(vm) {
        return el("div", data.text);
    };
}

i think this is a good balance and i can sleep at night with removing the auto-keying.

also, newDiff is technically available as vm._diff since it becomes the new cached values, so render can simply accept oldDiff rather than taking up another arg, but it's not as hygienic :/

@l-cornelius-dol
Copy link
Collaborator

where do oldDiff, newDiff

Given that this is a "0.1%" optimization, I would be fine with the diffs being extractable from vm in some fashion. For example:

function diffs() { 
    return { old: oldDiff, new: newDiff };
}

by only ever injecting vm, we should have a much cleaner api

I concur.

model is not a great name and i agree it's confusing

Either data or state work for me. Maybe leaning a just a tiny bit toward state because that's what I've been thinking it is... but I would very quickly get used to data too. Then again, calling it data to match vnode data is quite attractive. I could easily internalize the ideas of "the vm's app data" and the "vnode's app data". I do love consistency, as you've likely guessed.

@l-cornelius-dol
Copy link
Collaborator

i can replicate the current closured-model behavior in userspace

I like the explicitness of that much, much better.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 6, 2017

i tried dropping all closure and render args in eaef453 and the resulting ergonomics were not great IMO.

needing access to vm.data is so fundamental everywhere that adding an extra level of indirection is a real pain in the ass. you need to alias it everywhere to make sure you minify well and the code stays terse and readable. one thing that i always disliked about React examples is this.props.fullName when you could simply say person.fullName had the props been passed in as an arg and named logically by the end user. otherwise you have crazy deeper shit like this.props.person.fullName (shudder). react's props must live on this because classes & prototypes have no real private state, you only get this in a constructor/closure.

not passing data to the closure may be okay-ish to avoid it going stale, but not passing the always-current vm.data into render is a recipe for boilerplate aliasing or huge java-esque, incompressible verbosity. so the question becomes, what value does forcefully not passing it do vs just ignoring the extra args?

for me, keeping the closured model/data updated (if needed) from inside render() is the shortest (3 chars minified) and 0-overhead solution. if this method feels dirty to you, then you can always use vm.data as @tropperstyle has.

function View(vm, data) {
    function click() {
        data.count++;
    }

    return function(vm, newData) {
        data = newData;
        return el("a", {href: "#", onclick: click}, data.count);
    }
}

when plain-object views are implemented, then a closure will not be forced on the unwilling, and you won't have any of this staleness to worry about.

  • i'm on board for renaming model to data
  • removing the auto-keying would require adding notices for unkeyed views to
    • add keys if there are matching sibling views but of different data
    • reset any internal view state during a willUpdate hook (if needed)
    • update the closured data either from willUpdate or render() (if needed)

@l-cornelius-dol
Copy link
Collaborator

l-cornelius-dol commented Jun 6, 2017

adding an extra level of indirection is a real pain in the ass

Can the overarching problem not be resolved by not including the data in the View factory arguments, since it is a conflict with a stale value?

function View(vm) { // no data arg for "initial" value of data
    function click() {
        let data=vm.data; // only if you actually need data during event
        data.count++;
        }

    return function(vm, data) { // here's where you most often need data
        return el("a", {href: "#", onclick: click}, data.count);
    }
}

Personally, I would far prefer to employ explicit helper functions than have counter-intuitive "magic" and "strangeness". For me, caveats indicate possible flawed design. That is, the "data may be stale" indicates a possible error.

function View(vm) { // no data arg for "initial" value of data
    return function(vm, data) { // here's where you most often need data
        return el("a", {href: "#", onclick: vmHandler(vm,data,incCounter) }, data.count);
    }
}

function incCounter(vm,data) {
    data.count++;
}

function vmHandler(vm,data,hdl) {
    return hdl.bind(null,vm,dta);
}

This tends to result is looser coupling and more reusable with more explicit values and less assumptions. The code is easier to understand in context.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 6, 2017

// here's where you most often need data

right. that and event handlers (and maybe hooks). typically the data is not needed directly within the closure itself. maybe it makes sense to pass the current data as an arg to event handlers (as well as render) to prevent having to alias it there. everywhere else is much less common.

This tends to result is looser coupling and more reusable with more explicit values and less assumptions. The code is easier to understand in context.

btw, in your example, you're re-creating the onclick handler every redraw, which means domvm will re-bind el.onclick each time. onclick: [incCounter, data] is what you likely want, where incCounter(data, e, vnode, vm)

@tropperstyle
Copy link

FWIW I'm happy with the route this discussion took. More important than any semantics, is a strong conviction from the main contributor to shape the vision of the project and to stay true to that focus. I think mithril lost some of that, which is why die hard fans are now looking for alternatives.

It also looks like render was trimmed down to 4 args, which I think is much more palatable than 6.

Tangents:
Is the goal to not hit any domvm warning messages before shipping code?
Any thoughts on replacing babel for typescript?

@leeoniya
Copy link
Member Author

leeoniya commented Jun 10, 2017

I think mithril lost some of that, which is why die hard fans are now looking for alternatives.

Mithril's API never felt great to me. things like always-global redraw (without granular redraw) and its use of a Controller in pre-1.0 was the deal-breaker that had me look elsewhere. I found domchanger and it felt much better but ultimately pretty slow and still too restrictive. and so i decided to write a vdom lib that took some api bits i liked from both libs and here we are.

It also looks like render was trimmed down to 4 args, which I think is much more palatable than 6.

yep, i've never used key or opts in render and if i ever need them they're both available on the vm or in the closure's args since they're immutable. newDiff is technically the same as vm._diff, but i dont want to promote that ugly pattern.

Is the goal to not hit any domvm warning messages before shipping code?

ideally, yes. but not necessarily. for instance, you may want to define handlers ad-hoc via {onclick: e => alert("clicked " + e.target.id) }. that will show an INLINE_HANDLER warning because the handler is recreated and rebound on each redraw. it's not ideal but if the view is not perf-sensitive, then stylistically it's a bit cleaner than defining the handler outside of render for reuse.

you'll definitely want to avoid UNMOUNTED_REDRAW and MISMATCHED_HANDLER because they can cause undefined behavior in the lib. SVG_WRONG_FACTORY is a notice of incorrect api use. FOREIGN_ELEMENT, REUSED_ATTRS, UNKEYED_INPUT may cause unexpected behavior in the app.

Any thoughts on replacing babel for typescript?

domvm builds use Bublé rather than Babel.

as for typescript, i'm warming up to the syntax, albeit slowly. same for Prettier. i havent done too much testing for bundle size and resulting perf of TSC, so i'm not ready to migrate domvm to it yet. however, i'm open to someone contributing a domvm.d.ts public api definition file.

@l-cornelius-dol
Copy link
Collaborator

l-cornelius-dol commented Jun 10, 2017

FWIW I'm happy with the route this discussion took. More important than any semantics, is a strong conviction from the main contributor to shape the vision of the project

I completely agree, particularly with "shape the vision". I like having input, making my case... and I'm happy for Leon to say, "Yeah... nah!". Swings designed by committee very rarely work out well.

It's very hard to reason about things out of context as Leon's final comment showed, with respect to arg order, "it became obvious why the arg order for events was e, vnode".

Mithril's API never felt great to me.

Components, "controller", global redraw + redraw suppression are what disenchanted me, though the concept is great. But finally, the disengagement of Leo and the sense of "lost-ness" of the community coupled with an apparent desire to complicate the shit out of building it (even if simplicity of use was preserved).

But, what actually made me most nervous with Mithril is the sheer velocity of commits; it felt like this very simple, solid core on which to build an app was becoming the next Behemoth. I very much want to see DOMVM's core stabilize and remain largely untouched, while specific things develop around it's periphery.

i've never used key or opts in render

Neither have I.

as for typescript

I'm very much opposed to it; something like DOMVM should remain Vanilla JS. In no small part because "I very much want to see DOMVM's core stabilize and remain largely untouched", therefore, DOMVM should not need a meta-language on top of JS. Related is that I want to be able to debug POJS if ever there appears to be a problem in DOMVM.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 10, 2017

"I very much want to see DOMVM's core stabilize and remain largely untouched",

i assume you're talking about the core api. the only time when the core will remain untouched is when domvm is no longer actively maintained. i do hope that v3 will be the last api iteration for some time.

domvm v1 had aspirations for being 100% build-free. however, that didnt seem to provide the ease-of-contribution benefit it was meant to encourage. people don't even help write docs, lol.

in the end build-free isnt realistic in a complex, well-organized app. you'll always have at least some assembly of modules and domvm v1 left this up to the browser and acript includes, which moved the burden to the frontend/production, which was shitty.

domvm's codebase is not es5 either, and will likely use more es2015 features, not just modules. so the build steps are already there. they're easy but not absent. the build process produces both minified bundles and non-minified es5 bundles with sourcemaps that anyone can debug & tweak without building anything. what language the actual source is written in is only of relevance to those who work on the code, not users of the lib (who will always get es5 bundles). therefore, the aversion to typescript by users is rather misplaced given the current state of affairs.

that being said, i dont see much value that typescript would bring me, the only core dev. the project is small and digestible and i usually code in an editor that doesnt have TS support anyhow. maybe once vscode starts also working well for php, i'll consider switching. i do see a lot of value in a domvm.d.ts typings file so the API can be nicely consumed by users of domvm who prefer write their code in typescript and get the IDE benefits that come with that.

@tropperstyle
Copy link

write their code in typescript and get the IDE benefits that come with that

Precisely. No editing environment has lured me away from my trusty vi setup until now.

@lawrence-dol One of the goals of TypeScript is to emit idiomatic, recognizable JavaScript code[1]. No source code mappings necessary. Everything is essentially opt-in, so you can use it merely as a standard ES2015 compiler, which is how I got started.

I found immediate benefits from the implicit types, and eventually moved to VS Code to see the errors and warnings instantly on keystroke, instead of waiting for run time or even build time. Then came the light of inline type definitions, documentation, and source-code, and it is a whole new world. I no longer need to visit some terribly formatted website for documentation, because I can discover a library's API as I consume it, with each keystroke. If I am refactoring something, I can see right away if I forgot to rename a reference.

@leeoniya One of my immediate draws to domvm was its readability. Though it lacked a large community, I felt comfortable reading the source code and confident I could reason about it if necessary. Taking this a step further, if this project was in TypeScript, I would never need to leave my editor. While upgrading versions, mismatched API arguments would be highlighted instantly. I'm totally biased, but embracing types can be a solid form of documentation, and we know everyone loves documentation =)

[1] https://github.com/Microsoft/TypeScript/wiki/TypeScript-Design-Goals

@leeoniya
Copy link
Member Author

leeoniya commented Jun 12, 2017

While upgrading versions, mismatched API arguments would be highlighted instantly.

wouldn't a domvm.d.ts API typings file serve this purpose? creating one wouldn't require adopting TS in the core but still provides all the benefits to consumers of domvm's APIs

@leeoniya
Copy link
Member Author

leeoniya commented Jun 12, 2017

for v3, i'm going to drop ES6 class views in favor of plain object views. the didInit(vm) hook is being removed in favor of {init:...} return to align with this.

the main problem with ES6 class views is that the calling convention for all the defined methods needs to drop any vm arg in favor of this to be what users expect. this means testing in the core at each call site and that's not something i want to do. instead, plain object views will provide the same benefit that es6 class views do - method sharing without re-creating them from inside a closure. any kind of inheritance can just be composed in userspace via Object.assign() or whatever - as javascript promotes. i'll still set this === vm everywhere, but i dont think there's much point in using it since it'll be in the args and minify better than this. there were never real tests for es6 class views, and i'm sure if i were to write them, i would have discovered this issue sooner.

@leeoniya
Copy link
Member Author

plain object views are pretty sweet. and were basically 0 effort.

https://rawgit.com/leeoniya/domvm/3.x-dev/demos/object-views.html

@leeoniya
Copy link
Member Author

leeoniya commented Jun 14, 2017

what are people's thoughts on splitting the autoPx functionality (appending px to numeric css object props) from the bundles and leaving it as an optional include? there are a couple reasons i'm considering pulling it out of the builds.

  • autoPx adds perf overhead because every css property [of css objects] must be tested against an isNumeric check as well as a dictionary of unitless props
  • the dictionary takes up ~299 gz bytes and by being baked into the lib it's hard to tweak by end users without recompiling domvm.
  • if i must have inline css, i usually opt for short css strings rather than css objects.
  • when i do use css objects it's typically for performance gains of patching individual props, in which case it's faster to just append the "px" in userspace rather than testing every property and erasing some of the speed benefit.

you'd simply include src/view/addons/autoPx.js to restore the current behavior. it would would contain:

domvm.config({
    autoPx: (function() {
        var unitless = {
            animationIterationCount: true,
            boxFlex: true,
            boxFlexGroup: true,
            boxOrdinalGroup: true,
            columnCount: true,
            flex: true,
            flexGrow: true,
            flexPositive: true,
            flexShrink: true,
            flexNegative: true,
            flexOrder: true,
            gridRow: true,
            gridColumn: true,
            order: true,
            lineClamp: true,

            borderImageOutset: true,
            borderImageSlice: true,
            borderImageWidth: true,
            fontWeight: true,
            lineHeight: true,
            opacity: true,
            orphans: true,
            tabSize: true,
            widows: true,
            zIndex: true,
            zoom: true,

            fillOpacity: true,
            floodOpacity: true,
            stopOpacity: true,
            strokeDasharray: true,
            strokeDashoffset: true,
            strokeMiterlimit: true,
            strokeOpacity: true,
            strokeWidth: true
        };

        return function(name, val) {
            return !isNaN(val) && !unitless[name] ? (val + "px") : val;
        };
    })()
});

and the default impl would be

function autoPx(name, val) {
    return val;
}

EDIT

i did the impl on the 3.x-dev branch and it brings the lib size down to 5k gz, which is nice. however, i'm not a huge fan of requiring multiple includes to gain functionality; feels reminiscent of 1.x.

@l-cornelius-dol
Copy link
Collaborator

what are people's thoughts on splitting the autoPx functionality

If I've made any use of auto-px, it's accidental. I prefer to be explicit with this and most things. Carve it out, I say.

@iamjohnlong
Copy link

ditto.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 14, 2017

the current unitless list (as seen above) contains numeric props that will not have px auto-added. all the cases you mentioned are properly handled.

@leeoniya
Copy link
Member Author

i ended up with a compromise. i really don't want script includes and also don't want the smaller builds expanding. i moved autoPx up to the micro build and added domvm.defineElementSpread & domvm.defineSvgElementSpread to it.

@leeoniya
Copy link
Member Author

leeoniya commented Jun 15, 2017

xlink:href support has landed. i also brought back the DATA_REPLACED devmode notice since v3 will continue to pass vm.data into the closure.

while i may still do some internal cleanup, v3's features & API are now fully baked. if anyone feels adventurous, please take it for a spin and report any issues or perf regressions.

i'm gonna spend some time re-testing the demos, running the full benchmarks and updating the docs. barring any serious regressions, v3 should be tagged sometime next week.

@leeoniya
Copy link
Member Author

initial round of benchmarks shows a possible minor regression (~5%) in dom creation for massive trees (30k elements, 10k subviews). everything else falls within the noise variance. mem useage shows ~15% reduction. i think we're good for perf.

@leeoniya
Copy link
Member Author

almost forgot about domvm.streamCfg(). i'm gonna convert it to a domvm.config() option:

// old
domvm.streamCfg({
    is: null,
    val: null,
    sub: null,
    unsub: null,
});

// new
domvm.config({
    stream: {
        is: null,
        val: null,
        sub: null,
        unsub: null,
    }
});

@leeoniya
Copy link
Member Author

btw, i removed the flyd adapter from domvm core and moved it into userspace in the demos: https://github.com/leeoniya/domvm/blob/3.x-dev/demos/streams.html

@leeoniya
Copy link
Member Author

leeoniya commented Jun 22, 2017

SSR benchmarks [1] are in. this is for 204 views & 2007 DOM nodes

vidom             // 1630 ops/s
inferno           // 1430 ops/s
domvm             // 1246 ops/s
preact            //  604 ops/s
react.with-hack   //  228 ops/s
vue               //  100 ops/s
react             //   30 ops/s

code for the curious:

var domvm = require('../node_modules/domvm/dist/server/domvm.server.min.js');

var el = domvm.defineElement,
    vw = domvm.defineView;

function mkArr(count, fn, frag) {
  let i = 0, arr = [];

  while (i < count)
    frag ? arr.push.apply(arr, fn(i++)) : arr.push(fn(i++));

  return arr;
}

function App(vm, data) {
  return () =>
    el('.app', [
      vw(Header, data),
      vw(Content, data),
      vw(Footer, data),
    ])
}

function Header(vm, data) {
  return () =>
    el('.header', mkArr(data.childrenNum, i =>
      el('div', { id : 'header-' + i })
    ))
}

function Content(vm, data) {
  return () =>
    el('.content', mkArr(data.childrenNum, i => [
      el('b', 'bold' + i),
      el('span.link', [
        vw(Link, { href : '/', value : 'link-' + i })
      ]),
      el('i', 'italic' + i),
      el('div', [
        el('div', [
          el('div', [
            el('div', 'div')
          ])
        ])
      ])
    ], true))
}

function Link(vm, data) {
  return () =>
    el('a', { href : data.href }, data.value)
}

function Footer(vm, data) {
  return () =>
    el('.footer', mkArr(data.childrenNum, i =>
      el('div', { id : 'footer-' + i })
    ))
}

module.exports = childrenNum => () => domvm.createView(App, { childrenNum }).html();

[1] https://github.com/dfilatov/vidom/tree/master/benchmarks

@leeoniya
Copy link
Member Author

leeoniya commented Jun 26, 2017

k guys, i have a barebones playground working. please try it. i'd like to start moving all demos into it.

https://leeoniya.github.io/domvm/

@iamjohnlong
Copy link

Looks good man, it'd be nice to be able to resize the panes.

@leeoniya
Copy link
Member Author

lots of stuff would be nice :) the whole thing is 216 LOC [1]; contributions always welcome!

[1] https://github.com/leeoniya/domvm/blob/gh-pages/playground/index.html

@l-cornelius-dol
Copy link
Collaborator

@leeoniya

i assume you're talking about the core api. the only time when the core will remain untouched is when domvm is no longer actively maintained.

I guess what I meant was the conceptual core, which includes the API. I hope that the core of DOMVM becomes highly stable, and the out-of-the-box functionality is delivered as optional parts of an ecosystem, rather than DOMVM continually growing, in both complexity and size.

You've shown every proclivity, to date, in keeping the core small and laying in additional functionality. I think that's good.

@l-cornelius-dol
Copy link
Collaborator

By the way, I am using V3 for a new project. So far, so good. I quite like the ergonomics of the view returning an object, with the init and render functions outside the enclosure.

@leeoniya
Copy link
Member Author

I quite like the ergonomics of the view returning an object, with the init and render functions outside the enclosure.

sounds like you might as well just use plain object views:

const View = {
  init:   (vm)       => {...},
  render: (vm, data) => tpl,
};

@l-cornelius-dol
Copy link
Collaborator

of course, after fixing the demos yet again to work with the updates, it became obvious [1][2][3] why the arg order for events was [what it was] ... i'll likely revert back to ...args, e, vnode, vm, data (with the addition of data)

I completely agree having just ported Mithril withAttr to use a little event handler. In the context having the specified arguments first was perfectly natural.

@leeoniya
Copy link
Member Author

leeoniya commented Jul 2, 2017

🎉 v3.0.0 🎉

changelog: https://github.com/leeoniya/domvm/releases/tag/3.0.0

@leeoniya leeoniya closed this as completed Jul 2, 2017
@leeoniya
Copy link
Member Author

leeoniya commented Jul 3, 2017

heads up! event handler registration for vm.emit() has changed:

vm.on(evName, callback) -> vm.config({events: {evName: callback}})

sorry i had to do this API break post-3.0, but it was an oversight and this change makes things more uniform with other similar changes introduced in 3.0.

will be tagging 3.0.1 in a bit.

@leeoniya leeoniya mentioned this issue Jul 5, 2017
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