milo
milo copied to clipboard
milojs
Cannot create a simple class with item facet
The following setup complains of
Uncaught MixinError: method trigger already defined in host object
<ul ml-bind="[list]:todos">
<li ml-bind="Todo:todo"></li>
</ul>
var Todo = milo.Component.createComponentClass('Todo', ['item']);
milo.registry.components.add(Todo);
milo(function(){
milo.binder();
});
I see some clumsy solutions... Don't like them. The workarounds are either to explicitly list all required facets in class definition (also clumsy) or to put item facet in template: ml-bind="Todo[item]:todo". But you know it probably...
Yeah, I have been looking at it this morning.Should there not just be a different method for Facet$check to use which simply adds the facets to the component config... but I guess at this stage the config has already been run. Maybe something to iterate the facet config before the facets are instantiated, to look for requires. Hmm, not so simple.
It looks like it could one day be a source of trouble, look at all the places we use it. You can search the project for require: ['
There are 2 solutions
- We may pass an extra parameter to addFacet to prevent starting it in the case it is called from check. addFacet already has 4 parameters and in check it is called like addFacet(reqFacet)
So it will become
addFacet(addFacet, undefined, undefined, undefined, false);A bit ugly. - Another idea is to track facets that were started (e.g. assign this._started = true in Component$start, which is empty now) to prevent them started for the second time. so instead of using allFacets('start') in Component$init we can iterate them and only start those that are not started already. Probably a bit less ugly... But some of the facets may have forgotten to call inherited start (as it was empty anyway), in which case they may be started twice if they are required... Unlikely we will be affected by it though. So we need to pick one :) Any better ideas?
Or we can create another method addFacetWithoutStarting and call it from check... Maybe this one is the prettiest of all :)
Maybe the last, given that it would more or less be an internal method, not used elsewhere. I had thought of the others too. I don't mind 2 either.
actually, it is not working in some cases, as it makes, e.g., Data facet to be started before Container facet which it cannot do, as Container facet should be started before Data facet... Require enforces the order, but class declaration does not... We can rely on object order of course and simply re-order facets, but it is not good... And actually we are relying on it anyway :)
Even the facent._started idea is not so simple to implement because it needs to be checked in allFacets. I got it working by adding something horrible like this:
!facet['_'+method+'ed']
the fix is in the branch https://github.com/MailOnline/milo/tree/start-order, but more changes are needed before it can be merged. See #44
The change to allFacets method should be based on some parameter, as it is simply designed to run a certain method on all facets, regardless whether it was run before. getState uses it too for example. I think we should simply iterate without allFacets if we want to check anything.
The fix I did works, it just exposes a potentially bigger issue of us relying on object iteration order when we start facets. We need to define some start order (#44)
The example in examples/todomvc_class/index.html now loads without throwing, but the list simply doesn't work. I'll take a look.
As a temporary workaround, I suggest removing item from class definition and putting it in template... That's what we did in other places :)