TodoMVC in Mithril v2 or Where did controller() view(ctrl) go?

[space88man]

Requesting for TodoMVC guide for v2. Currently, the example uses

var app = app || {};
app.controller = function () {

and

app.view = (function () {
	var focused = false;

	return function (ctrl) {

		return [
			m('header.header', [
				m('h1', 'todos'), m('input.new-todo[placeholder="What needs to be done?"]', {

However this pattern isn't documented (last was in v0.2.5). The Simple application tutorial uses either
view: function(){...} or view: function(vnode){...}.

The view receives accepted or 'completed from the URL capture '/:filter' as the filter property on the ctrl argument populated by controller().

In modern mithril (>=v2)/Simple application tutorial it comes in by vnode.attrs.filter? The TodoMVC example still works on v2, how did the view function know it was receiving (old-style) ctrl or vnode?

in view: function(vnode){} I would expect the URL capture /:filter (filter = "active" or "completed") to appear as vnode.attrs.filter. However the received argument ctrl does not seem to be polymorphic (it does not have an attrs property).

[dead-claudia]

@space88man Could you link to the problematic example?

[space88man]

Here: https://github.com/tastejs/todomvc/tree/master/examples/mithril

Especially this file: https://github.com/tastejs/todomvc/blob/master/examples/mithril/js/controllers/todo.js which uses the controller() pattern. The Component here https://github.com/tastejs/todomvc/blob/master/examples/mithril/js/views/main-view.js uses ctrl as the received argument.

This code seems to still work so speaks to the backward compatibility of Mithril! It would be great to have an up to date repo under the MithrilJS org.

[dead-claudia]

Better would be to rewrite their example and send them a pull request to do it. It's been outdated since at least Mithril v1 was released and we just hadn't updated it yet. I do appreciate bringing it to our attention!

Edit: Marked this accordingly as a bug.

[space88man]

Great - thanks to your v1 comment I managed to find the migration guide (controller->oninit) at the v1.0 Change log. Guess this will be my weekend homework...

[dead-claudia]

@space88man There's similar for v0.2 → v2, if you want a shortcut.

[space88man]

@isiahmeadows - thanks.

I have done minimally invasive changes to the todomvc example here: https://github.com/space88man/todomvc-mithril2. RFC for improvements and incorrect use of Mithril 2 patterns.

Original example is linked further up in this issue.
Porting from 0.2 to 2.0.3:

• remove .controller()
• remove .withAttrs() and .prop() usage

[orbitbot]

@space88man 🎖

Noticed the following:

• I don't believe it's particularly meaningful to use an IIFE for the main component here
• the call to Data.init() here might as well be in the closure itself instead of as a separate lifecycle method
• this whole function is redundant, you can instead just inline the Data.visible(filter).map call
• I'm not really sure if the onupdate call here is at all meaningful or should just be removed
• this class definition might as well be inlined ( .destroy )
• using css to toggle visibility here might as well be done with !Data.isEmpty() && m('section.main', ...
• it would arguably be more correct to refactor the focused variable to be completely removed (since it's actually just called in the oncreate no matter what), a check against document.activeElement ( if (document.activeElement === vnode.dom ) or ideally remove theoncreate lifecycle with adding [autofocus] to the input
• it's not necessarily that much neater, but arguably the class parameter here could be refactored to the more concise class : '' + (task.completed ? 'completed' : '') + (task.editing ? ' editing' : '')
• the footer should/could be refactored into a component (m(Footer, { active, filter })), currently the Footer factory will get called on every redraw https://github.com/space88man/todomvc-mithril2/blob/master/js/views/main-view.js#L103
• the amountCompleted variable is not used in the footer and probably should, or be removed

As for the rest, I'm not sure if Data is something that has been provided by the TodoMVC example itself, but IMO this kind of layer should not itself be concerned with unpacking an attrs parameter or a dom event, so these should be managed in the component file instead;

(this is not comprehensive)

• this line affects the dom and doesn't change the data layer at all
• similarly add should get a clean title string as a parameter
• don't know just from reading the code if the filter processing actually works properly in the app, but in any case the attrs parameter should not be managed in a data layer
• ...

All in all, I didn't check what the TodoMVC dictates wrt. patterns or code provided, but the whole implementation could probably be made more concise with writing things from scratch.

[osban]

@space88man I agree with @orbitbot's "... the whole implementation could probably be made more concise with writing things from scratch."

Edit: I think the coding style has changed quite a bit since 0.2, so if you'd want to create an example using the latest coding ideas, then starting from scratch would be the best way, IMHO.

[space88man]

[UPDATE] Refactored, incorporating the comments, and converting components to closures.

Thanks for the comments: I'll refactor taking the tutorial as a guide rather than my too literal port of the 0.2 example.

Question:

1. for idiomatic v2 where do you store your state? Per the tutorial there is a User object that stores the list of users, plus the save and load methods. Following that pattern I created the Data object to hold the list of Todos, and helper methods, based on the previous controller() method.
2. @osban: "I think the coding style has changed quite a bit since 0.2" - is there a good guide (the tutorial ?) for the preferred v2 coding style? Using closure components, RouteResolvers perhaps?
3. Without using streams, what is the v2-way for doing the withAttr/prop type of "data binding". In the legacy example this was used quite extensively.

[osban]

@space88man we (@orbitbot, @StephanHoyer, and I) have been code golfing on the channel a bit, and my implementation (everything in one file) is this. A standalone working version is here. If you want to use the code you'll have to separate the code in modules and such (according the the MVC setup rules). Let me know if you have questions or comments 🙂

Edit: Of course you might find it more fun to write your own code instead of using (part of) mine, so do what you think is best 🙂
Edit: Made some changes and did some refactoring.

To answer your questions (didn't see them until now, sorry):
ad 1. My personal guidelines for state/components are: if there's no internal state - use a pojo; if there is internal state - use a closure; if there's shared state between parent-child - use attrs; otherwise use global state
ad 2. yeah, I think the tendency is to use more closures and ES6 features. I also think the MVC separation is not as strict as it used to be.
ad 3. oninput: e => variable = e.target.value

[osban]

Couldn't resist making a split version 😉
And if you want to be sure that only actions (controller) can modify the model: flems.

[space88man]

Awesome!! 😄 🙇‍♂️ 🙇‍♂️

@isiahmeadows please feel free to close - maybe add @osban( + others) work to https://mithril.js.org/examples.html? Thank you.

[dead-claudia]

I'll leave it open until TodoMVC itself is updated with a v2 implementation.

[osban]

@space88man I'll leave the honors of adding it to TodoMVC to you (you can also wait until @orbitbot and/or @StephanHoyer is ready with his version, and see if you like that one better).

[osban]

@space88man Found some bugs: some actions weren't saved properly. Here's the corrected version (only actions can modify model): flems.

[dead-claudia]

Filed tastejs/todomvc#2263 to try to fix this. Since there's nothing that can realistically be done in this repo (and this repo's independent TodoMVC clone differs wildly from the official repo's), I'm closing this issue.