MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 926 forks source link

Components don't work with ES6 classes. #618

Closed Naddiseo closed 7 years ago

Naddiseo commented 9 years ago

Since ES6 classes need to be called with new and this needs to be an instance of the class, components will not work with a class. The issue stems from here: https://github.com/lhorie/mithril.js/blob/next/mithril.js#L554

Example (using babel-compiled class): https://jsfiddle.net/1prjtv78/

A work around is to wrap the component controller like:

var Component = {
   controller: function () { return new ComponentController(...arguments); }.
   view: View
};

Which transpiles to something equally ugly:

var _bind = Function.prototype.bind;
var Component = {
   controller: function () { return new (_bind.apply(ComponentController, [null].concat(arguments)))(); }.
   view: View
};

Example of working: https://jsfiddle.net/1prjtv78/1/

Perhaps the parametize.controller function can do something similar to the transpiled code.

tobyzerner commented 9 years ago

You might be interested in how I've integrated ES6 classes with Mithril components in my project: https://github.com/flarum/core/blob/master/js/lib/component.js

Usage:

class FooComponent extends Component {
  constructor(props) {
    super(props);

    this.showing = m.prop(true);
  }

  view() {
    return m('div', this.showing ? this.props.content : '');
  }
}

m.mount(document.body, FooComponent.component({content: 'bar'}));

Note that the component class itself is the controller, and the view is its method. It's a bit more like React.

Naddiseo commented 9 years ago

That's some tricky/non-obvious code going on in component(), at first I couldn't remember what this refers to inside a static method, then I wondered "where does this.props come from?". But, interesting idea none-the-less.

An aside: I notice you don't allow parameters to the view function. I was arguing this with a co-worker today. I think a component's view should allow for arguments so that you can pass in a child vdom. For example, say you have a component that is logically a container, wouldn't you want to be able to pass in the child virtual dom to the component's view? An overly simplistic example:

class MyContainer {
   view(ctrl, children) {
      return m('div.cool-class', {}, children);
   }
}

function View(ctrl) {
   return m('div', [
      m.component(MyContainer, [m('div', "I am a child")])
   ]);
}

m.mount(document.body, {view: View});

As for using view as a method: I'm still undecided on this in my code for two reasons:

I do like being able to use this inside of the view though

barneycarroll commented 9 years ago

TL;DR: this isn't how Mithril works. Why would you want it to work this way?

The purpose of the components structure is to provide 2 objects — a controller constructor and a view function — such that Mithril internals can generate and store instances of the controller according to internal logic and pass those specific instances to the view function during redraw. The Mithril internals that depend upon this association take care of controller initialisation under the hood, so on the face of it whether or not a component is a constructor or not isn't a user-land issue, except for the fact that if the user were to generate new instances of a component whenever they invoked it in the view, you would expect the controller instance to be lost, what with a new object being consumed by the view — except it isn't, because Mithril manages association internally and the old controller instance would persist depending upon circumstances. Meanwhile, this consistently refers to the controller instance within the controller itself, meaning that any attempted access to this or super would be inconsistent.

On Wednesday, 13 May 2015, Richard Eames notifications@github.com wrote:

That's some tricky/non-obvious code going on in component(), at first I couldn't remember what this refers to inside a static method, then I wondered "where does this.props come from?". But, interesting idea none-the-less.

An aside: I notice you don't allow parameters to the view function. I was arguing this with a co-worker today. I think a component's view should allow for arguments so that you can pass in a child vdom. For example, say you have a component that is logically a container, wouldn't you want to be able to pass in the child virtual dom to the component's view? An overly simplistic example:

class MyContainer { view(ctrl, children) { return m('div.cool-class', {}, children); } } function View(ctrl) { return m('div', [ m.component(MyContainer, [m('div', "I am a child")]) ]); }

m.mount(document.body, {view: View});

As for using view as a method: I'm still undecided on this in my code for two reasons:

  • Extra level of indent
  • Separation of concerns (template vs logic) in the same file and same object.

I do like being able to use this inside of the view though

— Reply to this email directly or view it on GitHub https://github.com/lhorie/mithril.js/issues/618#issuecomment-101481957.

Regards, Barney Carroll

barney.carroll@gmail.com +44 7429 177278

barneycarroll.com

Naddiseo commented 9 years ago

@barneycarroll, to whom are you directing that comment? If it's me: mithril should be agnostic whether or not my "controller constructor" is a plain function, ES5 class/object, or ES6 class, but it shouldn't break in the case it is an ES6 class. My problem is that mithril changes this inside the component constructor with definitely is an issue as it breaks the assumption of any constructor no matter where it's a constructor for. Nothing about using an ES6 classes (or object in general) stops mithril from functioning the way you describe.

Meanwhile, this consistently refers to the controller instance within the controller itself, meaning that any attempted access to this or super would be inconsistent.

Without using this, how do you store state? And using closures isn't really an option since they're not as performant as using objects.

Edit:* changing this is also an implementation detail of mithril that should not leak through to user-land code. If I pass in a class constructor, it should work since it obeys the component signature: it looks like a duck, quacks like a duck, and walks like a duck, so why doesn't mithril treat it as a duck?

barneycarroll commented 9 years ago

@Naddiseo sorry, I just worked out what you're going on about. Bad reading on my part. The bug is that controllers don't inherit prototypes (classes are just a sugar expression that defines constructor + prototype in one).

Without using this, how do you store state?

Personally, I've never been a fan of this. The limitations, gotchas and convoluted code patterns surrounding scope application and prototypal inheritance have always been horrific. In many ways I'm disappointed ES6 gave us class because it makes things look simpler while preserving the flaky runtime. My controllers are factories. I use object literals and scope-agnostic functions wherever possible, and it makes like a lot easier (incidentally, everything is terser and you get private state):

function controller( x, y ){
  var privateX = x
  var privateY = y

  return {
    publicZ : function(){
      return privateX + privateY
    }
  }
}

And using closures isn't really an option since they're not as performant as using objects

I'd dread to think what kind of application you have that's facing measurable performance issues because of closures. You realise Mithril core doesn't use prototypes at all and the internal functions execute 1000s of times in any given view?

But I digress. For better or worse, this is expected to work with controllers. Prototypes are a core part of JS. We should fix this.

barneycarroll commented 9 years ago

Thought this would do it, but tests are breaking. :/

barneycarroll commented 9 years ago

@Naddiseo hey, I think this might be a false alarm caused by typos in the original fiddle (the controller's controller prototype method is never invoked) – could you check this out and see if it works for you?

Code, for reference:

let component = {
  controller : class{
    constructor(){
      this.x = 1
    }
    increment(){
      this.x ++
    }
  }, 
  view       : ctrl =>
    m('h1', 
      "Hello ", 
      ctrl.x,
      m( 'br' ),
      m( 'button', {
        onclick : e => ctrl.increment()
      },
        'Increment!' 
      )
    )
}

m.mount( document.body, {
  view: ctrl =>
    m('div', 
      component, 
      " World" 
    )
} )
ow-- commented 9 years ago

@barneycarroll But now you've changed the premise by not using m.component() for the parameterization. The assessment that prototypes are a core part and this should be fixed ought still to stand. This also breaks using a TypeScript class as a controller.

Anyway, if you leave args as a parameter to parameterize() rather than slicing arguments in your changeset the tests go through and it works as expected.

StephanHoyer commented 9 years ago

plz, plz make the use of classes optional. I don't want to be forced to use them. I find them cumbersome and ugly. The code is IMHO much easier to read without them.

barneycarroll commented 9 years ago

plz, plz make the use of classes optional

+100

The argument for seems to be "I can't experience terrible flaws in the language because the library doesn't give me the opportunity". It's not a credible user story if you ask me.

diogob commented 9 years ago

+1 to @barneycarroll and @StephanHoyer. this is a terrible feature, we're better of with lexical scoping. Simpler and more readable.

I would say that adding support for classes even as an optional feature is adding unnecessary cruft to the codebase.

But I'm also interested in hearing more from @Naddiseo and @tobscure why they want Components as classes.

Naddiseo commented 9 years ago

@diogob, I only want to be able to use classes for the controller portion of the Component. For the view portion I do what most other people do and use plain functions and pass in a ctrl arg. As for why classes:

  1. (Opinion) I find it easier to organise my code. And, I like scoping related functions with the data they operate on.
  2. (Opinion) I prefer to store data on this than passing around multiple arguments
  3. (Sort-of-fact) When I originally benchmarked the different styles of writing the controllers, using Object.create was slow, and using lexically scope function was comparable but slower to using classes It seems those benchmarks may no longer hold up, I'll create some other tests and see if my assumptions still hold water.

Whatever the case may be, my point earlier in the thread still holds: if mithril uses new on the controller, it should be unaware of where it's a function or a class, thus classes would still be optional. What this issue is about is that mithril breaks the assumptions I can make about the controller; mithril tells us that it is newed, so I would expect passing anything new-able would work.

syaiful6 commented 8 years ago

if you just want that class as your controller why not just pass it like this controller: {controller: yourClass, view: yourView, then i think it will work as you expect.

syaiful6 commented 8 years ago

ops, it will not work. sorry..

barneycarroll commented 8 years ago

@syaiful6 @Naddiseo I realise this is a huge point of irritation when trying to rationalise object schemas and instances. I've recently proposed a new syntax for Mithril components that might interest you in 499.

daslicht commented 8 years ago

One great thing about classes is that you have full code-completion, you dont have to remember any names just select them.

dead-claudia commented 8 years ago

@daslicht BTW, that point is moot if you consider that current components are almost classes conceptually.

IMHO, I see other reasons to integrate classes and/or functions, though. A great example is developer ergonomics and debugging at scale - think of the React Developer Tools extension for Chrome. It's pretty helpful for React developers. Without the ability to get the name of a component, it's not possible to create that kind of thing for Mithril. And if components are plain objects, that's not possible (objects don't have a name property like ES6 functions and classes do, nor do they have a displayName like functions in IE). Oh, and if a component returns the wrong thing (e.g. forgetting to return a node from your view, an easy and common mistake), you can at least use the name of the class to make the error message more descriptive, since stack traces won't help. You don't get that luxury with object literals.

@lhorie You might appreciate the above paragraph for your rewrite.

daslicht commented 8 years ago

I don't care how it's called unless you get full code-completion. That works currently even across files (at least with TypeScript)! So that you don't have to remember, or look up how things are named. Just write a Class one time and forget the naming of its members.

Just choose from a list without even tying. That makes meaningful names much faster to use than typing them manually 1000 times. If that workflow could be offered by functions I would use functions.

dead-claudia commented 8 years ago

@daslicht Can you choose from a list of everything in scope that satisfies a type? I don't use a lot of TypeScript or IDEs of any kind.

barneycarroll commented 8 years ago

@daslicht typing the same thing out 1000 times sounds like a lot of hard work. Can you point to any example of a meaningful name in you have typed out more than 10 times in the context of a Mithril component?

My contention is that "classes are awesome" is a truism that can't be related to this topic in a practical way. None of the advocates have mentioned any scenario where this might be of use except in the most general way possible: the onus is on people who've actually looked into the code and use it without classes to reimplement the API with classes so that people who like classes can see whether or not it's useful to them.

It doesn't sound like a very convincing or appealing exercise.

dead-claudia commented 8 years ago

@barneycarroll I did mention developer ergonomics - it's not possible to develop things like React Dev Tools or similar for Mithril, since object components don't have that magic name property functions and classes do. That kind of thing helps with larger web apps, if you're troubleshooting something with UI content that's off. (No, unit testing doesn't help you fix logic. It only tells you if the logic is broken.)

That might not appeal to you, since IIRC you don't use it much at scale, but I know some do, and that ability would be incredibly powerful for those use cases.

It also helps the more kinesthetic, tactile programmers as well, which tend to do best with a very powerful, detailed autocomplete and GUI graphs of all the nodes in the tree, moving in real time, like in Light Tables. IIUC, you're best with just text in that tab completion with identifiers and some mild autocomplete works perfectly fine for you most of the time, and pictures usually just get in the way of your usual routine. I'm more of a mixture, where I need a little of both. It really comes down to how people think (and how no two think alike).

So I understand how it wouldn't appeal to everyone, but since people think best in different ways (classes are a little more visually descriptive, while objects are a little more concise and easier to textually digest), and both versions have their merits (components are easier to generate from factories if they're objects, while classes are easier to statically analyze), that's why I've always supported both versions from the very start.

barneycarroll commented 8 years ago

I'll fill in the blanks using my imagination: if you were using Typescript with an IDE you would type m( and get a dropdown of all available components in your codebase. Plugging in the Mithril inspector would let you visually map components and their internal state at run time. Is this what we're after?

dead-claudia commented 8 years ago

@barneycarroll The former you should already be able to do now with most TypeScript IDEs (like Atom TypeScript or VS). The latter is mostly an optional thing. When I'm writing React code, I rarely consult the React Dev Tools, but I've seen a lot that really love it. As for the other side of the deal, some people conceptualize far better with classes instead. I've met several people that way.

Oh, and at some point, when you're writing at scale, and this is the key part you seem to be missing, there's a reason why Flarum wrote a wrapper for class-based components. They wanted a consistent structure for components, and this is usually where classes tend to excel. I do agree that there are a few things that could be simplified, but consistency and clear structure helps when you're writing a large app.

Would you prefer a slightly lower barrier of entry and increased scalability for those writing Mithril, and more people adopting and liking the framework, or do you want it to become very opinionated in favor of free-form components, in a non-mainstream way, potentially alienating existing users as well as making life difficult for those writing large Mithril apps?


And in direct reply to your comment:

I'll fill in the blanks using my imagination: if you were using Typescript with an IDE you would type m( and get a dropdown of all available components in your codebase. Plugging in the Mithril inspector would let you visually map components and their internal state at run time. Is this what we're after?

Only if you want it. I'm promoting enabling the option, not making that the default workflow. Neither of those will have native integration into core, and they never should. For what it's worth, even mithril-objectify isn't in core. Just because something exists doesn't mean you have to use it.

barneycarroll commented 8 years ago

alienating existing users as well as making life difficult for those writing large Mithril apps?

I'm lost. What's the alienating difficulty we're proposing? There seems to be a subtext that I develop Mithril applications that aren't large enough — people who like classes are presumably building larger and more important things. I don't really know how this inference was made or how it relates to the subject under discussion. I was going to say "specifics under discussion", but we seem to be drifting further and further away from any chance of that.

You're absolutely correct in saying Mithril Objectify isn't core. None of the Mithril plugins I developed to support my app are core, either. Again, I don't know how to tie this back to the perceived problem or what a solution might look like.

Let's take a step back. I came into this thread to try and work out what the petitioners are asking for in practical terms, and wondering what work could be done to alleviate those issues — specifically, my contention is that "we want classes" as a movement doesn't seem to have any clear developer user stories that we might use as qualifications of success in trying to develop a solution to this issue.

I'm promoting enabling the option

Can you go into more detail about this?

ondreian commented 8 years ago

I'm currently using ES6 classes as components, mainly for separation of concerns and ease of debugging for complex situations (D3 visualizations or a dynamic HABTM menu for example)

Here is a basic example for this: codepen

If mithril were to change to detecting prototype in component or whatever, and then constructing the Class I think this fundamental change to the API would make it harder to reason about using classes. Currently this is easily handled via class static methods, and I would prefer to keep explicitly constructing my classes in a uniform way.

However, if you feel the feature is worth pursuing it might be prudent to detect if the class accounts for Mithril wanting controller and view properties before constructing the instance?

Logically, I find it much more transparent to think of the instance as the current controller's internal state, than the component's instance, and the view should be static relative to the controller instance.

I'm not a zealot, I just though I'd weigh in on how i'm currently using classes myself, and a few others I've converted are using mithril with es6.

pdfernhout commented 8 years ago

Related comments by me on other ways to do components in Mithril: https://github.com/lhorie/mithril.js/issues/733#issuecomment-201945393 https://github.com/lhorie/mithril.js/issues/499#issuecomment-201949762

dead-claudia commented 8 years ago

@ondreian

Actually, it would be a simple typeof component === "function" check to see if it's a class (arrow functions would fail to construct, and would not initially be valid components). That simple. Pure components will complicate the picture, but it's undecided if they're even necessary, considering the following helper (I personally use a variant of this now):

// function pure(func: (...args) => VirtualNode)): Component<any>
const pure = func => ({view: (_, ...args) => func(...args)})

const TagList = pure((post, isTag, resolvedTag) => m(".post-tags", [
    m("span", "Tags:"),
    post.tags
    .map(tag => [tag, isTag && tag === resolvedTag ? ".post-tag-active" : ""])
    .map(([tag, active]) => m(`a.post-tag${active}`, route(`/tags/${tag}`), tag)),
]))

(Yes, I'm more of a functional programmer.)

dead-claudia commented 8 years ago

Edit: fix bug in plugin

And in a conversation I had with @barneycarroll in Gitter privately, one thing I came up with for how ES6 components could feasibly be supported now: use this wrapper function.

function classToComponent(C) {
    return {
        controller: function () { return new C(...arguments) },
        view: (ctrl, ...args) => ctrl.view(...args),
    }
}

If you want to try this now, you might be able to use this Mithril wrapper plugin (it's in CoffeeScript for algorithmic clarity, but it's easily translated into JS):

classPatch = do ->
  'use strict'

  classToComponent = (Component) ->
    controller: (args...) -> new Component(args...)
    view: (component, args...) -> component.view(args...)

  coerce = (func, self) -> (ctrl, args...) ->
    if typeof ctrl == 'function'
      func.apply self, [classToComponent(ctrl)].concat(args)
    else
      func.apply self, arguments

  classPatch = (old) ->
    m = coerce(old, undefined)
    # If you're unfamiliar with CoffeeScript, this is their `for ... in` loop
    for own prop of old
      m[prop] = old[prop]
    m.component = coerce(old.component, old)
    m

# Usage:
m = classPatch(m)
daslicht commented 8 years ago

@barneycarroll Regarding Classes ind Mithril you might be rirgth. I dont use Mithril yet so I cant tell much about it and dont have any example. It is ages ago i worked wuth real classes, last Time it was as I coded with Adobe Flex years ago. There we had full refactoring of Names across the whole project, even imports.You could move files and rename and anything stayed valid. We created value objects which represents for example a user andautocomplete allowed us to just choose values. So you had to implement it once and were abe to use it in as many new projects you like and never had to type any of its properties by hand. That is just one example, probably that makes it more clear what I meant by tyimg 1000 times.

@isiahmeadows Yes autocomplete just shows the public members and not all globals.

dead-claudia commented 8 years ago

@daslicht There's a few people, though, that would love to see classes make it into core, though. One big example would be with Flarum, where they themselves monkey-patched Mithril to work with almost React-like components (they use JSX + a couple internal Mithril plugins), and my proposed change would probably mostly only affect these two files (if they choose to take advantage of this), if I'm reading their code base correctly.

tobyzerner commented 8 years ago

@isiahmeadows Flarum dev here. Having classes work out of the box would be nice, although our implementation differs slightly from the patch that you've provided, in that we do some setup to store args on the component instance (so we can have this.props like in React). I don't think that's necessarily a good paradigm though... certainly if Mithril came with built-in support for classes, we'd look at adapting our components to fit in with the way it does things so we can run without a patch.

We've got some changes in the pipeline regardless (extracting our component paradigm into an external package), so now would be a great time for this to happen :D

dead-claudia commented 8 years ago

@tobscure

  1. You would still be able to store arguments on the instance and use that specifically:

    class Component {
     constructor(props, ...children) {
       this.props = props
       this.children = flatten(children)
     }
    
     view() {
       return m(".whatever", [
         "Do something with ", this.props, " and ", this.children,
       ])
     }
    }
  2. I did find that it wouldn't necessarily mean removing all the boilerplate. You still might need to patch it some, but you wouldn't need to patch m(). You'll still need some non-trivial boilerplate in your Component abstract class, though, given how you've architected the components.

The way I would be implementing this would be by effectively duplicating the functionality of this function to work with classes.

dead-claudia commented 8 years ago

Actually...I just realized a hitch...I need to also wrap the components on m.mount and friends as well... :(

This just got complicated.

pdfernhout commented 8 years ago

@isiahmeadows I like your classToComponent wrapper function. As a reminder, this can also be done with static methods directly on the class. I do that with TypeScript but I would think it should work the same in ES6.

 class StoryBrowser {
    ...
    static controller(args) {
        return new StoryBrowser(args);
    }

    static view(controller, args) {
        return controller.calculateView(args);
    }
    ...
dead-claudia commented 8 years ago

@pdfernhout I'm aware. I'm working on implementing it in core, though. Somehow, it's actually resulting in simpler code apart from this dispatcher function (trying to avoid an expensive Function.bind call):

function constructWithArgs(C, args) {
    // Try to fast-path with a direct call first.
    switch (args.length) {
    case 0: return new C()
    case 1: return new C(args[0])
    case 2: return new C(args[0], args[1])
    case 3: return new C(args[0], args[1], args[2])
    case 4: return new C(args[0], args[1], args[2], args[3])
    default: return new (C.bind.apply(C, [undefined].concat(args)))()
    }
}
pygy commented 8 years ago

The gitter discussion is moving too fast right now...

To differenciate reliably between functions and classes, you may want to provide a base class with a sentinel and check for its presence as an inherited property...

function Component(){}
Component.prototype.sentinel = {}

if (Foo.x ===Component.prototype.sentinel) { return new Foo() }
class MyComponent extends m.Component {}

m(MyComponent)

AFAICT it should work with both native and transpiled classes (either Babel or Bublé)

Edit... or simply using instanceof.

Edit2: I now see @isiahmeadows suggested something similar in the chat:

@lhorie Here's how React checks pure components: it checks to see if the function has a prototype and is a React.Component subclass. That is much simpler to do, but it's not trivial. Another idea is to check to see if the function has a view instance method, which is required for components.

Edit 3: too fast, not too fart :-/

lhorie commented 8 years ago

@pygy I was hoping for something that doesn't require a base class, but in the absence of such a solution, that's actually a pretty elegant alternative

dead-claudia commented 8 years ago

@lhorie @pygy

Edit: corrected a couple things

One of the things in the Gitter discussion I mentioned (I think...it was moving really fast) was this:

function isPureComponent(C) {
    return C.prototype != null && typeof C.prototype.view === "function"
}

I also suggested, for the rewrite, you can implement a layer of abstraction like below. You would, instead of working on the component directly, operate on its respective wrapper by using getWrapper(vnode.tag). Otherwise, it's practically a drop-in replacement. This could go on the component itself as well.

// I'm using ES6, but it's trivial to translate to ES5.
const wrapperMap = new Map()

function getWrapper(component) {
    if (wrapperMap.has(component)) {
        return wrapperMap.get(component)
    } else if (typeof component === "function") {
        return new FunctionWrapper(component)
    } else {
        return new ObjectWrapper(component)
    }
}

class ObjectWrapper {
    constructor(component) {
        this.component = component
        wrapperMap.set(component, this)
    }

    oninit(vnode) {
        vnode.state = {}
        this.component.oninit(vnode)
    }

    oncreate(vnode) { this.component.oncreate(vnode) }
    onupdate(vnode) { this.component.onupdate(vnode) }
    onbeforeremove(vnode) { this.component.onbeforeremove(vnode) }
    onremove(vnode) { this.component.onremove(vnode) }
}

class FunctionWrapper {
    constructor(C) {
        this.component = component
        this.pure = C.prototype != null && C.prototype.view != null
        this.tree = null
        wrapperMap.set(component, this)
    }

    oninit(vnode) {
        if (this.pure) {
            vnode.state = {}
            this.tree = (0, this.component)(vnode)
        } else {
            vnode.state = new this.component(vnode)
        }
    }

    oncreate(vnode) { if (!this.pure) vnode.state.oncreate(vnode) }
    onupdate(vnode) { if (!this.pure) vnode.state.onupdate(vnode) }

    onbeforeremove(vnode, done) {
        if (this.pure) return done()
        vnode.state.onbeforeremove(vnode, done)
    }

    onremove(vnode) { if (!this.pure) vnode.state.onremove(vnode) }
}

In ES5, the map could be implemented with a pair of arrays and an LRU cache (I'd tier it a few times to help larger apps), but this should provide the gist. (@lhorie, apologies if I wasn't clear enough :wink:)

syaiful6 commented 8 years ago

I propose the component passed to Mithrill must be a callable (pure function). This function just a factory, i call this component factory.

Component factory is a pure function, and should return an object wheter is plain object or instance of class with same signature as current Mithrill. Mithrill should call this function normally without new keyword. If the function not return what we expect like there are no view attributes, Mithrill can complain loudly (ie throw Error). Or if in the way Mithrill call this function an error occured let it be, so the developer know how to deal with it.

That way it help the library simplify the API, and not require user to extends base class or bothering whether the component is pure function or classes.

We dont need to know the function passed to us are pure function or not, just call it, if there are no error occured then check the object signature returned. If errror occured let it be, ie Babel transpilled code throwing Error.

Example taken from Mithrill

Current API:

var MyComponent = {
    controller: function(data) {
        return {greeting: "Hello"}
    },
    view: function(ctrl) {
        return m("h1", ctrl.greeting)
    }
}

I propose:

function my_component() {
    return {
        controller: function(data) {
            return {greeting: "Hello"}
        },
        view: function(ctrl) {
            return m("h1", ctrl.greeting)
        }
    };
}

That way, people with like to write them as classes can do something like:

function my_component() {
    return new MyAwesomeClass();
}

For people using classes, maybe write function just to instantiate their class are really too annoying. To solve this issue Mithrill can provide a function decorator for them, something like:

function decorateClass(cls, ...args) {
    return new cls(...args);
} 

Deprecation:

For people that write a component based plain object like the sample code on Mithrill home, we can detect this. So we can wrap them to a pure function for purpose of this new API. Mithrill can log the message to console so we know it going to removed in the future.

dead-claudia commented 8 years ago

@syaiful6 My proposal for the rewrite can be very easily refactored to permit that as well.

(By the way, most of the design work is going into the rewrite, since it's already here. Feel free to check it out, but do be aware it's far from production-ready.)

 class FunctionWrapper {
     constructor(C) {
         this.component = component
         this.pure = C.prototype != null && C.prototype.view
+        this.stateful = !this.pure
         this.tree = null
         wrapperMap.set(component, this)
     }

     oninit(vnode) {
         if (this.pure) {
             vnode.state = {}
-            this.tree = (0, this.component)(vnode)
+            const ret = (0, this.component)(vnode)
+            if (this.stateful = (ret.view != null)) {
+                vnode.state = ret
+            } else {
+                this.tree = ret
+            }
         } else {
             vnode.state = new this.component(vnode)
         }
     }

-    oncreate(vnode) { if (!this.pure) vnode.state.oncreate(vnode) }
-    onupdate(vnode) { if (!this.pure) vnode.state.onupdate(vnode) }
+    oncreate(vnode) { if (this.stateful) vnode.state.oncreate(vnode) }
+    onupdate(vnode) { if (this.stateful) vnode.state.onupdate(vnode) }

     onbeforeremove(vnode, done) {
-        if (this.pure) return done()
+        if (!this.stateful) return done()
         vnode.state.onbeforeremove(vnode, done)
     }

-    onremove(vnode) { if (!this.pure) vnode.state.onremove(vnode) }
+    onremove(vnode) { if (this.stateful) vnode.state.onremove(vnode) }
 }
syaiful6 commented 8 years ago

That's great.. i dunno it already rewrite, hmmm, i will take a look. of course i dont use them on production!

There are a library implements lru cache, but we can write ourself for this purpose. thanks..

dead-claudia commented 8 years ago

I will note that the rewrite is backwards-incompatible.

dead-claudia commented 8 years ago

@lhorie was working on it for a while, and once it got here, of course a lot of us were eager to look at it.

lhorie commented 8 years ago

For rewrite, I'd like the design for this proposal to be top-down first, and be informed by implementation details second.

In my mind, if ES6 classes are to be supported, the ideal syntax should be:

//definition
class Foo {
  view() {return "hello"}
}

//consumption
m(Foo)

As for implementation: ideally, this should also be possible

//definition
function Foo {
  return "hello"
}

//consumption
m(Foo)

The issue is that a) both classes and functions have the same type and it's generally not possible to distinguish between them without calling them or peeking at its toString(), b) they have different calling restrictions (classes must be called w/ new and arrows must not be called without new, and c) calling a primitive-returning function w/ new loses the return value

The idea of using a base class is the closest to the ideal so far. Ideally, we need a non-hacky isClass implementation of some sort that passes these tests

lhorie commented 8 years ago

For the record, from some quick benchmarking, peeking at toString appears to be 2 order of magnitude slower than calling the function

pygy commented 8 years ago

What do you look for in the toString() output? I'm afraid you can detect a class compiled by Bublé by looking at the source, where

class Foo{
  view () {
    return "hello"
  }
}

becomes

var Foo = function Foo () {};

Foo.prototype.view = function view () {
  return "hello"
};
"function Circle( radius ) {
    Shape.call(this);
    this.radius = this;
  }"
lhorie commented 8 years ago

toString can tell you definitively that the thing is a ES6 class or an arrow, though it doesn't help with other edge cases

darsain commented 8 years ago

Please don't even consider checking classes with toString. So slow. Wouldn't this be enough?

let result;
try {
  result = fn(); // fails when class
} catch (e) {
  result = new fn();
}
let rendered = result && result.view ? result.view() : result;
Naddiseo commented 8 years ago

Adding my 0.02c:

Enforcing components to extend from a base class seems too opinionated for mithril, so I would be -1 for that solution. Without another alternate, I think @syaiful6's solution is better since it's allowing us the flexibility to decide how to structure the code by giving us an interface (a function that returns something with a view function).

As an aside, I definitely prefer components that are plain JS objects. I think I've missed some of the discussion here (I don't follow gitter), why do components have to be callables?

lhorie commented 8 years ago

They don't have to be, but if it's low complexity, I don't mind adding support for class-based components, since some people like them.

Pure function components are nice-to-have because they require less typing and can be composed by FP-oriented folks

Class-based components are nice-to-have for the more OOP-oriented folks.

One use case that is currently not very nice is component methods. Right now they have to be procedurally attached to vnode.state. I was thinking of pointing this in lifecycle methods to vnode.state to make that more tractable, but declaring methods in the component object declaratively a la React.createClass is currently not supported. It would be nice if it was, and if we could support classes with the same code, it would be even better.