-
-
Notifications
You must be signed in to change notification settings - Fork 923
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
Subcomponent Controller access to latest attributes #683
Comments
I think this behaviour is OK. You are essentially making the items array a part of the component's state, and state should not change unless you explicitly tell it to. My solution would be: var ItemManager = {
controller: function (attrs) {
var ctrl = this;
ctrl.items = attrs.items;
ctrl.createItem = function() {
ctrl.items.push( new Item() )
}
},
view: function (ctrl, attrs) {
ctrl.items = attrs.items; // explicitly update state each time new attrs are passed in
return m('.items', [
m('button', { onclick: ctrl.createItem }, "Create Item"),
attrs.items.map(...)
])
}
} OR var ItemManager = {
controller: function (attrs) {
// don't make the items array a part of the state; require that
// it's passed through from the view
this.createItem = function(items) {
items.push( new Item() )
}
},
view: function (ctrl, attrs) {
return m('.items', [
m('button', { onclick: ctrl.createItem.bind(ctrl, attrs.items) }, "Create Item"),
attrs.items.map(...)
])
}
} |
Yes, those are the workarounds I have had to do. I argue that they are non-obvious to someone new to Mithril, and the debugging path to these workarounds takes quite an effort. |
There is also the cognitive overhead of figuring out what you can and cannot do with |
I'm curious to see the alternatives you have in mind? |
A few ideas:
|
There are several alternate implementations of the The issue stated differently: during a redraw, if Mithril sees a parameterized component in the same DOM position as the previous redraw, and it has the same base component, it is considered to be the same component and the cached controller is reused: even if the arguments are different. However, even though it is considered to be the same component, the new arguments are passed to the view function. Here is a JsFiddle which demonstrates the issue; the view keeps seeing the new value, but the controller is cached once and never updated. This behavior is unusually subtle and a bit frustrating, and I think a lot of developers are going to encounter this peculiarity; I certainly ran into it very quickly. In order to suggest a solution, I investigated the origin of this behavior. In actuality, every invocation of // Without the convenience, components must be created in parent controller.
var ComponentsInController = {
controller: function(){
return {
subComponent: m.component('myComponent', 'args');
}
}
view: function(ctrl){
return m("", [
ctrl.subComponent;
]);
}
}
// With the syntax convenience, components can be created in the view and the controller is still cached.
var ComponentsInView = {
view: function(ctrl){
return m("", [
m.component('myComponent', 'args')
]);
}
} While that syntax is admittedly useful in cutting controller boilerplate, the logic used to determine that two components are the same is too fuzzy; if the arguments to a component change, it should be considered a different component. In fact, the "correct" behavior is already available in one case: if the So what is the solution? I think there are three options:
I think that any other behavior is going to be difficult to reason about, and only prove frustrating as more developers try to pick up the framework. |
@mrtracy I respectfully but strongly disagree with the notion that component controllers don't reinitialize enough. These should be used for initialisation. It is expected and extremely useful behaviour to be able to cheaply pass new data through to the view without having to reinitialize the controller. I have my disagreements with the use of I'd be happy to offer tailored suggestions for any case in which controller reinitialization is deemed to be expected, but AFAIC this is a definite case of user error. |
I've just read through some of your discussions with lhorie about In light of that, I think there is an education problem. In the API documentation for m.component, the majority of the parameterized component examples consume the passed arguments in I think in the basic API examples, users should be primarily encouraged to consume the arguments directly in the I also think that a non-trivial example should be created of a scenario with the following requirements:
It seems like such a component would have some arguments that change, but other arguments that are static (needed for the expensive initialization). If the arguments needed for the initialization will change, it seems you would need a |
I think part of the problem is my internal ideals of controller responsibilities. One of those responsibilities is providing actions (methods to call) for the view. In a parent controller, I want to be able to give ItemManager an array of items and have it do whatever it wants with it. As @barneycarroll suggested, this is possible with a plain function. However, React and Mithril have gotten me thinking in components; to me, Mithril's component features are not just a useful tool for managing the render cycle, but also for code organization. At the moment, according to my (possible incorrect) ideas of what a controller should do, it's unfortunate that I have to treat the controller differently based on whether I'm working with a top-level component or a child component. Plain functions are useful, sure, but if I want a well-defined, independent section of the page, I prefer not need to choose between a Mithril component or a JS function. The crux of the issue is hard to put into words. I'm still kind of figuring out how to express it. |
Another point: I had thought I found a simple go-to pattern: Start by putting actions and state in the controller, and refactor out as necessary. It seems that I would have to follow @barneycarroll's suggestion no. 3, or start with an already-factored out state and actions. For the record, a |
To borrow some concepts from other languages, passing So, to add to the options @tobscure presented, you could also wrap the argument in a m.prop, so that you're able to replace the array inside of the getter-setter function while maintaining a "pointer" to it (similar to how C pointers work) |
@lhorie Actually I have used that technique before. I ran into an issue at that point, too:
It took me a long time to debug this issue as well. The proper solution to this problem is to never recreate the view-model, and instead iterate through the original and set all the properties from the server. This requires me to write more code, and disallows me to reuse |
I too have been tripped up by this, and my conclusion is that the abstraction is currently broken. I suspect that the arguments should be considered arguments to the view only, and not passed to the controller, but I cannot yet be certain of this. What I am certain of is that the current formulation is badly broken and that the solutions/work-arounds presented in this thread are merely band-aids. That people are suggesting "work-arounds" to Mithril's current behavior is proof that it's broken. |
Though this did initially catch me by surprise, I found it does force me to reconsider how I am modeling my data, and so far it has always led to a better separation of concerns. As far as removing args from the controller function, I currently use the following pattern all the time. Basically it allows the nested components to retain the UI state edited values. The edit widget values are restored to the underlying model data every time the controller runs. Using a key with an id + updated timestamp usually will do the trick. var EditItemWidget = {
controller(args) {
this.title = m.prop(args.title);
this.description = m.prop(args.description);
},
view(c, args) {
return m('.edit-input-widget', [
m('form', [
m('input', { type: "text", value: c.title(), onchange: m.withAttr('value', c.title) }),
m('textarea', { value: c.description(), onchange: m.withAttr('value', c.description) }),
m('button', { onclick: e => args.onchange(c.value()) }, 'Save')
])
]);
}
};
var Application = {
controller() {
this.items = m.request('/path/to/items');
}
view(c) {
return m('div.items', [
c.items().map(function(item) {
var key = [item.id, item.updated_on].join();
return m.component(EditItemWidget, Object.assign({ key }, item)
})
]);
}
} |
@tropperstyle : The problem with this pattern is it breaks encapsulation with respect to the container and component in that the container now needs very specific knowledge of the component implementation in order to know that it needs such as key as is being provided. In principle, the container should not require such specific knowledge. It's brittle and fragile and does not lend itself well to fully encapsulated black-box components which I safely "just use". |
I understand what you are saying. It seems like the issue boils down to the
I was off-put by this too, but are there better suggestions out there? I agree components should be as fully-encapsulated as possible, but at the same time it is still necessary to have control over the rendering and initialization life cycle. |
One solution I had in mind was to access attrs via the controller itself:
This way Mithril can set But this requires Mithril to settle on a strict signature for the component:
...which I personally think would be an excellent standard to agree upon, but I know it might not settle well with others (aside from the fact that it's a breaking change). |
It's not so much Mithril or the component mechanism that's broken as the understanding of data flow in this particular application. Back when we were discussing how to make inline component invocation work (half a year ago?), this was the model decided upon. When we suggest that Gilbert's code reveals broken internals, it's important to consider what would be happening instead if this snippet were to work as expected: the controller function would need to be invoked every time the draw executed. At that point, the distinction between controller and component is merely a useful semantic aid for authors but has no practical function – you may as well make this a view only component since you expect all the code to execute every redraw. Let's not forget the pre-0.2 days when people were doing just that by accident and getting horribly confused. |
I don't mean broken functionally, I mean the abstraction is broken. There is a cognitive disconnect with respect to arguments and the controller and the view. |
Yeah, got that. What I'm saying is that the expectation is wrong – or, to put it more bluntly, the cognitive disconnect is a failure to understand the necessary lifecycle implications of controller vis-a-vis view. Here's the smallest tweak to the code to match original expectation: var ItemManager = {
view: function (ctrl, attrs) {
ctrl.createItem = function() {
attrs.items.push( new Item() )
}
return m('.items', [
m('button', { onclick: ctrl.createItem }, "Create Item"),
attrs.items.map(...)
])
}
} |
What I'm getting at is that the handling of arguments for components is weird; part of my component, the controller, sees only the initial arguments, and part, the view, sees the current arguments. That's a failure in the principle of least surprise. It might be technically correct if you think carefully about the life-cycle of the controller and view, but it's still surprising. Your "fix" constantly recreates the What I think is that it's indicative of a conflation of a parametrized component and a parametrized view. If the component is created in the view the view arguments are always fresh but the controller arguments are from the first invocation of the view which put that particular instance there. Components are currently incorrectly defined and/or designed. Where should I use |
I always understood that controllers only initialized once. The problem is that it's difficult to consider (or even put into words!) the subtle implications of that when writing a parent-child component interaction with changing data. |
I could add a section on the docs talking about the perils of consuming controller arguments (thereby creating a stateful component) when you really meant to write a stateless component. The docs about stateful/stateless components go some of the way to illustrate the difference in construction between the two, but clearly this nuance got lost in translation. I don't think there should be different conceptual entities to deal with the differences between a initialization parameter, vs a refresh parameter, and I don't think controller arguments should automagically get in sync w/ view arguments (on the grounds that it breaks common assumptions about closures and arguments in js, and makes testing ridiculously convoluted) |
I think another part of the issue is this line of thinking:
Even if you manage to document this very well, I'm pretty sure developers are still going to run into this issue. Here's another part of the issue. All this only matters when you're using a component as a subcomponent. Top-level components don't experience this issue, making controller methods completely ok.
For the record, the proposal I made completely sidesteps closures on function parameters. |
@lhorie : Is it not possible that controllers should not even be passed these arguments? Or does that mean that useful constructs are prevented? Though, binding arguments in the controller seems no more useful than binding them in the component constructor. So far as I can tell at this point, it looks like they are simply an error prone complexity with no real value. |
@lawrence-dol here's an example that requires arguments to be passed to the controller: http://lhorie.github.io/mithril/mithril.component.html#parameterized-initial-state |
@gilbert constructors are an incredibly confusing part of the JavaScript language at the best of times. It's pretty clear to me that the
AFAICT all this confusion is down to the 'pass by reference' behaviour: The assertion that this couldn't happen with a top level components is technically correct, but a total red herring IMO. At the top level, you'd have to declare/instantiate the parameters consumed by the component outside of the component hierarchy. If the reference to
@lawrence-dol the typical example of a component sees a controller making an HTTP request and passing a reference down to the view. You want the controller to run once in initialisation, but you want the view to run whenever anything had changed. This is the functional distinction between the two. My application code is deeply reliant on this distinction, but that may not be the case for your. It may be that you want to execute everything all the time, in which case you don't have any real need for a controller separate from the view - but personally I think is really important to keep it. @mindeavour mentions that keys aren't appropriate to his situation - but according to Mithril logic this is how it works: if you want to initialise a new component in the place of an old one, you use keys. It happens that keys are also the mechanism by which unique DOM elements are identified - personally I think that's a conflation of concerns, and I developed a pattern whereby I could control component lifecycle separate to the DOM. But between the two, there's no practical problem here. |
@barneycarroll I shared this same sentiment here, but haven't thought up an alternative API. Care to share your code? |
@tropperstyle sorry, I read that and then somehow lost it thinking it was another thread. The API is Modulator. It's a bit clunkier (invocation consists of 2 back to back variadic functions: 1 specifying the parameters for component identification, the next passing its arguments through to the component itself), and it invokes components with exactly the same signature this thread holds to be problematic. I've also just developed an API to fill in for initialisation and teardown hooks in config without keying if that rocks your boat. |
Mithril gives more responsibility to the developer than most other frameworks. My code tends to be 20% Mithril, 80% plain old javascript. Very little magic. Most developers come to Mithril with "ghost patterns" in their heads left over from the last framework they worked with. It makes us nervous to have to build our own scaffolding with original ideas. There are several very smart people in the Mithril community who take very divergent approaches. All I can do is share my "Mithril Philosophy". My models are simple. They usually grab data from the server, maybe filter/cull that data, and provide CRUD functions. Done. I think of the controller as a boot loader. It initializes the component and is never heard from again. I take @lhorie's suggestion of thin controllers seriously. My controllers call model functions as necessary (m.request), and initialize the view-model (vm.init). My controllers don't define new functions or initialize state. I can customize newly initialized components through the attrs parameter. The attrs may change later, but the controller doesn't care. It's done. My views do no work of their own. They build templates, end of story. They get/set state from the view-model. They get/set data through view-model functions. They may pass attrs to view-model functions where the work is done. They don't access the model directly. Many Mithril devs neglect the view-model. It's my workhorse. I treat the view-model as the API between my data model and the view. Every member of the view-model object interacts with the view. It may also access the model, but only by request of the view. It's in an outer scope where other views in other components can access its API. The This arrangement has never failed me. I like it because the role of model/view-model/view/controller is clearly spelled out. I've created a jsbin, attempting to address several issues in this thread. It's best viewed in external window with console open in devtools (not jsbin console). http://jsbin.com/likeca/3/edit?js,output
Maybe someone working on more complicated projects can point to where this system can go wrong. |
I've said I already knew controllers initialize once. I already knew why this problem is happening. That's not the issue here. I'm arguing it's too easy to run into this problem because of how subtle it is. You can consume controller arguments all the time and never run into it except in that one strange case, and suddenly you don't know why your code isn't working. It's starting to look like controllers should basically never touch the |
I use MyComponent.controller = function( attrs ){
if ( !attrs.userPermissions ) attrs.userPermissions = "limitedAccess"
model.addPrivateDataToAccessibleModelData( attrs.userPermissions )
viewModel.init( attrs.translateLanguage )
}
You could put this functionality in your component views, but if the data will not change in the life of your component, you will wastefully be re-initiallizing static data with every call to your component view. In the case of |
@pelonpelon Those examples are good, but they rely on the assumption that the data will never change. While that fact may be true today, future code changes could make it false on a later day. When than happens, you will run into this subtle bug and probably have to restructure your component. |
@mindeavor I was actually considering omitting that from the components documentation as well and only introducing it at the time that the caveat is explained. |
@lhorie I think that's the best thing to do. I will update my tutorials as well. |
@mindeavor Of course, you're right. I am an independent contractor and usually control both back and front ends. When you are faced with similar cases to the ones I explained above, where do you introduce one-time customization of a "generic" component? |
@pelonpelon : I have been putting such one-time customization in the constructor of the component itself, rather than in it's controller construction. For example:
|
@lawrence-dol : That's perfectly reasonable. I may try too hard sometimes to make all my logic fit into MVVC fences. I like neatly packaged things 😉 |
So come three months later, I'm reworking my Mithril tutorial, and reached the point where I need to explain how to do this "component delegation". I still feel like there isn't a great solution. To each reading this thread, how would you explain how to do this delegation to a Mithril newcomer? |
…relying on VMs more and more, due to a discovery I made about the statefulness of controllers. More on that here: MithrilJS/mithril.js#683 Anyways, things are looking much nicer now. I've refactored the Card and CardVMs to combine them, destroyed some controllers, and generally tidied things up. Now the restart works! Also, I fixed a bug where the uncommon Slime was not named correctly, causing a crash when attempting a lookup.
hey, i have a child component the code looks this was var Timeline = {
controller: function() {
return {
timeline: timeline
};
},
view: function(ctrl) {
return (
m('div', {class:'Timeline'},
m ('div',{ class: 'row all-width'}, [
m('section', { class: 'column'
},[
ctrl.timeline.map(function(entry, item) {
return m.component(TimelineItem, {
key: item,
title: entry.title,
})
})
])
])
)
)
}
};
var TimelineItem = {
view: function(entry) {
return m("article", {class: "row"}, [
m('div', {class:'column column-50'}, [
m('time', new Date(entry.date)),
m('h3',
m('a', {title:entry.title, href:entry.url}, [entry.title, browser.pageWidth, browser.topOffset])
)
])
])
}
};
const browser = {
topOffset: 0,
}
window.addEventListener("scroll", function(e) {
browser.topOffset = pageYOffset;
m.redraw() //notify view
}) how this behavior could be managed in order to render item component only one time inside it's parent ? |
@cmnstmntmn hi, I don't think you're talking about the same issue here. Fancy asking us in the chatroom, or in the email group — maybe filing a new issue if you think there's a problem with Mithril? |
Rewrite branch now uses |
Take the following code:
I have too often come across the limitation of
ItemManager
's controller only having access to the first version ofattrs
. For example:ItemManager
passes in an array of items:a.
m.component(ItemManager, { items: itemArray })
ItemManager
adds toitemArray
.ItemManager
generates a new value for itemArray, and passes that value to the sameItemManager
subcomponentattrs
, creating a new item will now add to the old itemArray. The new itemArray remains unaffected.This is a hard issue to explain, and the workarounds are non-obvious. As it stands, trying to modify the
attrs
parameter in the controller constructor breaks the principle of least surprise.I think Mithril should have some built-in way to allow the controller to have access to the latest
attrs
. I have some options in mind, but before I present them I want to make sure the problem is clear. Thoughts?The text was updated successfully, but these errors were encountered: