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

Components don't work with ES6 classes. #618

Closed
Naddiseo opened this issue May 12, 2015 · 72 comments
Closed

Components don't work with ES6 classes. #618

Naddiseo opened this issue May 12, 2015 · 72 comments
Assignees
Labels
Legacy: v0.2 For anything specific to v0.2

Comments

@Naddiseo
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@barneycarroll
Copy link
Member

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
#618 (comment).

Regards,
Barney Carroll

barney.carroll@gmail.com
+44 7429 177278

barneycarroll.com

@Naddiseo
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Member

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

@barneycarroll
Copy link
Member

@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--
Copy link

ow-- commented May 22, 2015

@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
Copy link
Member

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
Copy link
Member

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
Copy link

diogob commented Oct 1, 2015

+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
Copy link
Contributor Author

Naddiseo commented Oct 1, 2015

@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
Copy link

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
Copy link

ops, it will not work. sorry..

@barneycarroll
Copy link
Member

@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
Copy link

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

@dead-claudia
Copy link
Member

@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
Copy link

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

@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.

@winterland1989
Copy link

Most of the discussion at this point is whether to natively support classes themselves (not merely instances of them) as components in core, side-by-side with objects. It'd be closer to m.mount main, Demo what I've been suggesting.

May i ask why? As far as i can see, you are suggesting we should instantialize class automatically inside m.mount, but how do you inject initial model/state without change m.mount's signature?

@lhorie
Copy link
Member

lhorie commented May 18, 2016

@winterland1989 via vnode.attrs and vnode.children, e.g. m(MyComponent, {foo: "bar"})

@winterland1989
Copy link

i see, but this will change m's semantic, since m is supposed to be evaluated eagerly without cache(without subtree retain optimization), if we instantialized a stateful component using m, there must be some magic should be cast, no?

@tivac tivac mentioned this issue Jun 7, 2016
22 tasks
@dead-claudia dead-claudia removed this from the 0.3 milestone Jun 17, 2016
@dead-claudia dead-claudia self-assigned this Jun 17, 2016
@dead-claudia
Copy link
Member

Working on this now for the rewrite.

@dead-claudia dead-claudia added this to the Rewrite milestone Jun 18, 2016
@spacejack
Copy link
Contributor

Adding a few ¢ to this thread.

For stateless components, using a POJO is totally fine; I don't see much reason to use classes. So this will mostly be about stateful components.

For stateful components that have their own properties or methods, I think POJOs are slightly confusing when you first start using them, as they are used by Mithril as a template to create instances. What this means in a component method took some getting accustomed to. Using a class as a component may be a bit more intutive to beginners (at least for those who come from class based languages.)

For the rest of this comment I'm going to talk about Typescript, which I realize must represent a tiny minority of Mithril users, so I'm not sure how much effort should be spent accomodating us. Typed JS variants do seem to be gaining popularity however and I think these create some additional use cases for classes.

First, it's a little more conventional to declare a class to take advantage of types than to do something like this. Not that it's a terrible solution but it does have drawbacks.

Now that Typescript supports the rather nice feature of non-nullable types (as Flow does) it can be awkward to declare a non-nullable property on a typed POJO if you can't assign it until oninit runs. Example:

interface MyComponent extends Mithril.Component {
    foo: Foo
}

const myComponent: MyComponent = {
    foo: undefined, // <- can't do if we want non-nullable, can't omit either.
    oninit (this: MyComponent, vnode: Mithril.Vnode) {
        this.foo = new Foo(vnode.attrs.fooInitializer)
    },
    view() {/*...*/}
}

In the above case, I want to treat foo as non-nullable because I know it will always have a value, but Typescript can't really know that oninit gets called first (sort of like a constructor.)

Now interestingly, neither Typescript or Flow can actually detect this problem:

class MyComponent implements Mithril.Component {
    foo: Foo // this.foo is undefined but no compiler error
    constructor() {}
    oninit (vnode: Mithril.Vnode) {
        this.foo = new Foo(vnode.attrs.fooInitializer)
    }
    view () { ... }
}

So that would compile happily. Simply allowing classes to be used as components gets around the problem of having non-nullable property assignment deferred until oninit. For now anyway. There is the desire to fix that problem in future, so this workaround may not work forever.

What might be a better, long-term solution is to have the constructor called instead of/in addition to oninit and be provided with the Vnode.

class MyComponent implements Mithril.Component {
    foo: Foo
    constructor (vnode: Mithril.Vnode) {
        this.foo = new Foo(vnode.attrs.fooInitializer)
    }
    view () { ... }
}

For reference, here are types for mithril 1.0 that I've been hacking away on.

https://github.com/spacejack/mithril.d.ts

@pygy
Copy link
Member

pygy commented Sep 24, 2016

Folks, I'd love to have your feedback on #1339

You can play with it here: https://github.com/j2css/mithril.js/blob/b581e5a1b4843eca3937835b62714b160dcb6b8c/mithril.js

For classes:

class MyComponent extends m.Component {
  constructor(vnode) {
    // initialize things here
    // vnode.state doesn't exist yet, but you can access its future incarnation as `this`
  }
  // define hooks here if you want
  oncreate() {}
  view() { return "Classy" }
}
m.render(document.body, m(MyComponent))

For factories:

function MyFactory(vnode) {
  // initialize things here (don't rely on `vnode.state` yet)
  var state = {
  // define hooks here if you want
    oncreate() {},
    view(vnode) {
      // this === state === vnode.state
      return "Industrial"
    }
  }
  return state
}
m.render(document.body, m(MyFactory))

In both cases oninit is redundant but preserved because it would take more code to avoid calling it for non-POJO components. It is always optional.

@JAForbes
Copy link
Collaborator

@pygy that factory example is very interesting. It would be very nice to have a native way to write mithril components that don't need this or vnode.state.

I always imagined the various hooks to be passed in to the factory as streams, and for the factory to return a view stream and nothing else. But I think I like this better ( though I like hooks as streams as an idea ( I'd like if the router used streams too) ).

It would be nice to be able to return either a state object or simply just a view. Most components I'll write won't use any hooks, so wrapping the response seems a shame.

So this: would be a valid component

function MyFactory(vnode) {
  return () => "nice pun!"
}

m.render(document.body, m(MyFactory))

And because props are functions, you could return a view stream

function Adder(vnode){
  const a = m.prop(0)
  const b = m.prop(0)

  const sum = m.prop.combine( () => a() + b(), a,b)

  const view = sum.map(function(){
    return [
       m('input[type=number]', { oninput: m.withAttr('value', a) })
       ,m('input[type=number]', { oninput: m.withAttr('value', b) })
       ,m('Sum: ', sum() )
    ]
  })

  sum(0)

  return view
}

m.render(document.body, m(Adder))

I'd really like to be able to just return a function optionally, I think it will be the common case and could open up some interesting optimizations down the line if the view is a stream.

But I could happily live with the current proposal of just returning state too.

@spacejack
Copy link
Contributor

spacejack commented Sep 25, 2016

Here's a typescript example using a class component:

https://github.com/spacejack/mithril-class-test

And a factory component:

https://github.com/spacejack/mithril-factory-test

Experimental typings included. Seems to work okay so far. 👍

@jacksonrayhamilton
Copy link

Thanks @pygy for implementing #1339, especially for the component factory support. I was using factories for my React "classes" and am looking forward to having the same capability in Mithril. I'll take your code for a spin in my project and let you know if I have any issues.

@spacejack
Copy link
Contributor

I've updated my above examples to include POJO, class and factory components here:

https://github.com/spacejack/mithril-component-types

This uses my most recent type definitions for 1.0 with some additons to cover the class and factory component types (see here.) These definitions provide much better inference for POJO and factory components.

Class components are unfortunately a bit awkward to work with. I can't see a way to get strongly typed attrs in the hyperscript function (example) due to the difficulty of expressing the attrs & state types in the parameters. For a class component, hyperscript must take a typeof type which can't be generic.

There are pros and cons to each type of component, and then each of those can utilize different approaches to declare types (or not.)

@dead-claudia
Copy link
Member

@dead-claudia
Copy link
Member

@pygy has an outstanding PR to fix this in #1339. Just bringing a little more attention to it.

@lhorie lhorie removed this from the 1.0 (Rewrite) milestone Jan 8, 2017
@lhorie lhorie removed 1.x labels Feb 4, 2017
@tivac tivac added the Legacy: v0.2 For anything specific to v0.2 label Feb 7, 2017
@dead-claudia
Copy link
Member

Closing as we are no longer accepting feature requests for 0.2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Legacy: v0.2 For anything specific to v0.2
Projects
None yet
Development

No branches or pull requests