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

component parametrized with m.component() get its arguments list growing on every redraw #638

Closed
rielsi opened this Issue May 29, 2015 · 9 comments

Comments

Projects
None yet
6 participants
@rielsi

rielsi commented May 29, 2015

var comp = {view: function(ctrl) {
    console.log(Array.prototype.slice.call(arguments));
}};
m.mount(document.body, m.component(comp, "test"));
setInterval(m.redraw, 1000);

Got in console:

[controller, "test", Array[1]]
[controller, "test", Array[1], Array[1]]
[controller, "test", Array[1], Array[1], Array[1]]
[controller, "test", Array[1], Array[1], Array[1], Array[1]]
...etc...

"Pure" component aren't affected, it only happens if wrapped with m.component().

@venning

This comment has been minimized.

Show comment
Hide comment
@venning

venning Jun 11, 2015

Contributor

I'm looking into this, but don't have an answer yet.

On the surface, the problem is here (line 561):

function parameterize(component, args) {
    var controller = function() {
        return (component.controller || noop).apply(this, args) || this
    }
    var view = function(ctrl) {
        if (arguments.length > 1) args = args.concat([].slice.call(arguments, 1))
        return component.view.apply(component, args ? [ctrl].concat(args) : [ctrl])
    }
    view.$original = component.view
    var output = {controller: controller, view: view}
    if (args[0] && args[0].key != null) output.attrs = {key: args[0].key}
    return output
}

Each call to view internally is supplied with [ {} ] as an additional argument, at least in your example. The problem is that parameterize keeps appending the additional supplied arguments onto args before applying them to the view function, so each run of view gets all of the previous runs' arguments as well.

Options:

  • This might actually be expected behavior, but I can't imagine any view function actually parsing for all those ever-increasing arguments.
  • Possibly, there is some buggy behavior supplying that [ {} ].
  • Only the current execution of view should have the supplied argument appended.

The third option seems most likely to me, which would look something like:

function parameterize(component, args) {
    var controller = function() {
        return (component.controller || noop).apply(this, args) || this
    }
    var view = function(ctrl) {
        var currentArgs = arguments.length > 1 ? args.concat([].slice.call(arguments, 1)) : args
        return component.view.apply(component, currentArgs ? [ctrl].concat(currentArgs) : [ctrl])
    }
    view.$original = component.view
    var output = {controller: controller, view: view}
    if (args[0] && args[0].key != null) output.attrs = {key: args[0].key}
    return output
}
Contributor

venning commented Jun 11, 2015

I'm looking into this, but don't have an answer yet.

On the surface, the problem is here (line 561):

function parameterize(component, args) {
    var controller = function() {
        return (component.controller || noop).apply(this, args) || this
    }
    var view = function(ctrl) {
        if (arguments.length > 1) args = args.concat([].slice.call(arguments, 1))
        return component.view.apply(component, args ? [ctrl].concat(args) : [ctrl])
    }
    view.$original = component.view
    var output = {controller: controller, view: view}
    if (args[0] && args[0].key != null) output.attrs = {key: args[0].key}
    return output
}

Each call to view internally is supplied with [ {} ] as an additional argument, at least in your example. The problem is that parameterize keeps appending the additional supplied arguments onto args before applying them to the view function, so each run of view gets all of the previous runs' arguments as well.

Options:

  • This might actually be expected behavior, but I can't imagine any view function actually parsing for all those ever-increasing arguments.
  • Possibly, there is some buggy behavior supplying that [ {} ].
  • Only the current execution of view should have the supplied argument appended.

The third option seems most likely to me, which would look something like:

function parameterize(component, args) {
    var controller = function() {
        return (component.controller || noop).apply(this, args) || this
    }
    var view = function(ctrl) {
        var currentArgs = arguments.length > 1 ? args.concat([].slice.call(arguments, 1)) : args
        return component.view.apply(component, currentArgs ? [ctrl].concat(currentArgs) : [ctrl])
    }
    view.$original = component.view
    var output = {controller: controller, view: view}
    if (args[0] && args[0].key != null) output.attrs = {key: args[0].key}
    return output
}
@rielsi

This comment has been minimized.

Show comment
Hide comment
@rielsi

rielsi Jun 12, 2015

Thank you. It certainly looks buggy and "leaky" to me. I settle down on stuffing all parameters into the second argument (which "is meant to be used as an attribute map and should be an object"), so I can deliver arguments on to arbitrary level of a deeply nested components tree. It's also more conscious to have those arguments explicitly named.

rielsi commented Jun 12, 2015

Thank you. It certainly looks buggy and "leaky" to me. I settle down on stuffing all parameters into the second argument (which "is meant to be used as an attribute map and should be an object"), so I can deliver arguments on to arbitrary level of a deeply nested components tree. It's also more conscious to have those arguments explicitly named.

@pelonpelon

This comment has been minimized.

Show comment
Hide comment
@pelonpelon

pelonpelon Jun 12, 2015

Contributor

It looks like your solution puts a stop to the recursive calls to view (internally).
http://jsbin.com/noxecu/1/edit?js,console ( w/ modified mithril.js = http://output.jsbin.com/casote/9.js )

Make a pull request and see what Leo thinks.

But we're still left with a circular controller.
I don't know. Is that a problem?

Contributor

pelonpelon commented Jun 12, 2015

It looks like your solution puts a stop to the recursive calls to view (internally).
http://jsbin.com/noxecu/1/edit?js,console ( w/ modified mithril.js = http://output.jsbin.com/casote/9.js )

Make a pull request and see what Leo thinks.

But we're still left with a circular controller.
I don't know. Is that a problem?

@venning

This comment has been minimized.

Show comment
Hide comment
@venning

venning Jun 15, 2015

Contributor

@pelonpelon, what do you mean by "circular controller"?

Contributor

venning commented Jun 15, 2015

@pelonpelon, what do you mean by "circular controller"?

@mrtracy

This comment has been minimized.

Show comment
Hide comment
@mrtracy

mrtracy Jun 16, 2015

Contributor

I believe that m.component was designed to be always called inside of another view() function. Each time the containing view() function is called, a completely new "parameterized" object would be instantiated; thus, the expectation would be for each "parameterized" object to have its view method called only one time.

However, when passed to m.mount(), the parameterized object is now having it's view method called repeatedly; this was perhaps an unexpected usage of m.component, thus the bug.

Contributor

mrtracy commented Jun 16, 2015

I believe that m.component was designed to be always called inside of another view() function. Each time the containing view() function is called, a completely new "parameterized" object would be instantiated; thus, the expectation would be for each "parameterized" object to have its view method called only one time.

However, when passed to m.mount(), the parameterized object is now having it's view method called repeatedly; this was perhaps an unexpected usage of m.component, thus the bug.

@venning

This comment has been minimized.

Show comment
Hide comment
@venning

venning Jun 16, 2015

Contributor

Thanks, @mrtracy. If that's the case, then that explains a lot. However, I can foresee non-contrived use cases for wanting to parameterize a component and then mount it, so maybe the documentation should specify not doing that. There are ways to make parameterized components break on mount to prevent this usage, notably not creating a { view: ..., controller: ... } object inside of parameterize.

Either way, the functionality shouldn't be constantly growing the argument array it applies to view.


Okay, so is there an earlier discussion (maybe in a GitHub issue) that explains a little more of how Mithril approaches components? The implementation and documentation seem not quite done yet and I don't know how to approach helping.

To be clear, I am extremely grateful that Mithril adopted components, but I'm just not sure if the design or implementation is still in flux.

Contributor

venning commented Jun 16, 2015

Thanks, @mrtracy. If that's the case, then that explains a lot. However, I can foresee non-contrived use cases for wanting to parameterize a component and then mount it, so maybe the documentation should specify not doing that. There are ways to make parameterized components break on mount to prevent this usage, notably not creating a { view: ..., controller: ... } object inside of parameterize.

Either way, the functionality shouldn't be constantly growing the argument array it applies to view.


Okay, so is there an earlier discussion (maybe in a GitHub issue) that explains a little more of how Mithril approaches components? The implementation and documentation seem not quite done yet and I don't know how to approach helping.

To be clear, I am extremely grateful that Mithril adopted components, but I'm just not sure if the design or implementation is still in flux.

@tivac

This comment has been minimized.

Show comment
Hide comment
@tivac

tivac Jun 16, 2015

Member

@venning http://mithril.js.org/mithril.component.html

Note that the signature for m.mount calls out that it accepts a component, not a parameterized component (the result of calling m.component()). It seems like this is more of a documentation/nomenclature issue than a code one.

Member

tivac commented Jun 16, 2015

@venning http://mithril.js.org/mithril.component.html

Note that the signature for m.mount calls out that it accepts a component, not a parameterized component (the result of calling m.component()). It seems like this is more of a documentation/nomenclature issue than a code one.

@venning

This comment has been minimized.

Show comment
Hide comment
@venning

venning Jun 17, 2015

Contributor

@tivac, actually, I think that proves my point. On the same page, it states that

calling m.component(MyComponent, {foo: "bar"}) will return a component that behaves exactly the same as MyComponent, but {foo: "bar"} will be bound as an argument to both the controller and view functions.

The documentation seems to indicate that there is no practical difference between a component and a parameterized component, which makes sense in terms of how I would use them. I don't want to be geared up for components and then decide that something needs to be stuffed in to their parameters and then be forced to rewrite my codebase to handle such a small semantic difference.

On a philosophical note, subtle distinctions between seemingly identical concepts is exactly what I dislike about Angular in practice. Based on his writing, I would imagine @lhorie would also like to reduce the "cognitive load" of having to distinguish from small things like whether or not a component is or isn't parameterized.

Contributor

venning commented Jun 17, 2015

@tivac, actually, I think that proves my point. On the same page, it states that

calling m.component(MyComponent, {foo: "bar"}) will return a component that behaves exactly the same as MyComponent, but {foo: "bar"} will be bound as an argument to both the controller and view functions.

The documentation seems to indicate that there is no practical difference between a component and a parameterized component, which makes sense in terms of how I would use them. I don't want to be geared up for components and then decide that something needs to be stuffed in to their parameters and then be forced to rewrite my codebase to handle such a small semantic difference.

On a philosophical note, subtle distinctions between seemingly identical concepts is exactly what I dislike about Angular in practice. Based on his writing, I would imagine @lhorie would also like to reduce the "cognitive load" of having to distinguish from small things like whether or not a component is or isn't parameterized.

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Nov 23, 2016

Collaborator

Closing, since it's fixed now.

Collaborator

isiahmeadows commented Nov 23, 2016

Closing, since it's fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment