ciscoheat / mithril-hx

Haxe externs for Mithril (JS MVC framework)
181 stars 12 forks source link

this in module.controller() #11

Closed benmerckx closed 9 years ago

benmerckx commented 9 years ago

I've noticed mithril calls a controller method with new in JS, setting this to a context inside the controller method.

var controller = new (module.controller || function() {});

https://github.com/lhorie/mithril.js/blob/next/mithril.js#L520

This means the following does not work:

class Test {
    var context: String;
    public function new(context: String) {
        this.context = context;
    }
    public function controller() {
        trace(context);
    }
}

When used as a module this would throw an error because context is not found in this in JS. Do you think we could use the macros to always bind the controller method with the module object?

ciscoheat commented 9 years ago

Hello, that will be done automatically if you let the class implement one of the interfaces described in the Readme file for this repository.

benmerckx commented 9 years ago

I must have missed this somehow, I was sure I tried before :) Works correctly when implementing Module

ciscoheat commented 9 years ago

Ok, glad it works! :) I will clarify that it's a requirement in the documentation.

freakinruben commented 9 years ago

I'm experiencing a similar issue with accessing this in the controler, while implementing Mithril or Component.

When I try the example of https://github.com/ciscoheat/mithril-hx#from-scratch, it gives me the error in view that user.name is undefined. Is this something that used to work but changed or maybe broke?

benmerckx commented 9 years ago

Ruben, I've had the same problem when updating the externs but using an older version of mithril. When using mithril >2 it worked for me.

freakinruben commented 9 years ago

Hey Ben, thanks for your quick reply. Do you mean using the dev builds of mithril, or is there another branch somewhere with mithril > 2

freakinruben commented 9 years ago

I think I know why it doesn't work for me, the patch to bind this to the module/component only works when you use M.mount on the component. Since my component is either returned by another view or instantiated in a route, the patch doesn't have any effect on them..

@ciscoheat is there a way to always patch this always? Or otherwise clear up the documentation that a view should only return View and never use Component + only use View in routes?

ciscoheat commented 9 years ago

Hi, I haven't had any similar problems with routes or components that only uses a view method. The patch is required when using M.mount, that is the place where the controller method is instantiated: https://github.com/lhorie/mithril/blob/master/mithril.js#L598

I haven't seen a need for it anywhere else, but if you have some code, please paste it here or in a gist, and I'll take a look.

freakinruben commented 9 years ago

It happens when you want to nest components and the nested component has a controller: https://gist.github.com/freakinruben/c6dd102af2f1cc1a9ce7

If you run the above gist, you'll get the error: Uncaught TypeError: this.testVar is not a function in the controller of TestComponent. According to the mithril documentation is ok to nest components so that's why I'm a bit confused.. http://lhorie.github.io/mithril/mithril.component.html#nesting-components

ciscoheat commented 9 years ago

Thank you, it took some digging but now a few things are sorted out! Version 0.25.3 is now available on haxelib.

First, from the documentation it looks like components are only supposed to be nested when used inside an M.component call: http://mithril.js.org/mithril.component.html#nesting-components

If you upgrade the haxelib and add that call to your example, it should work now, like this: https://gist.github.com/ciscoheat/4f43644592b6b3d49c38/1dbaee26af15603d910ab590b9b10d6ab5b6eabc#file-gistfile1-hx-L17

Creating objects/components like that in view has a disadvantage however. They will be re-created on every redraw (which can happen many times per second), so I'd only recommend that for very simple objects. Another related issue is that the Mithril controller is as good as identical to a Haxe constructor. So it's better and cleaner to avoid using controller and use the ordinary constructor instead. I will make that the recommended path and reflect that in the documentation soon.

Here's the example, without controller: https://gist.github.com/ciscoheat/4f43644592b6b3d49c38

freakinruben commented 9 years ago

That's quick, thanks! I'm was still figuring out what the roll of controllers were in mithril-hx.

ciscoheat commented 9 years ago

No problem, let me know if you have any problems with it. I had to release version 0.25.4 though, to avoid a cast if you want to use M.component directly in the view method. This balance between type-safety and flexibility... :)

M.component(input) is similar to the controller/constructor issue actually. It basically returns a {controller: input.controller || noop, view: input.view} object (as seen here in the source), and if the input is an Haxe object (without a controller method, as suggested) that is redundant. You might as well use input.view() instead. I will probably add that to the documentation too.

@andywhite37, you had some problems in #7, this is slightly affected but very low chance it will break. (I tested with your example code as well.) Just a heads-up.