philtoms / mithril.elements

Adds composable custom element types to Mithril
MIT License
42 stars 4 forks source link

Uninvasive API variation #2

Open barneycarroll opened 9 years ago

barneycarroll commented 9 years ago

I use Browserify, and some aspects of the way elements treats modularity bother me. In particular, the over-reliance on reference by string and the possibility for overwrites (of registered and native elements, of mithril core) and I was wondering what you thought about a tweak that allowed an alternative usage pattern that ditches string registration and instead relies on direct reference.

The model I have in mind would make m.element return (the equivalent of) a partially applied call to m( element ). In practice, this means that a string reference to the element becomes redundant. This would allow the following scenario:

// elements/select.js
var e = require( 'mithril.elements' );

module.exports = e( controller, view );

// app.js
var m      = require( 'mithril' );
var select = require( 'elements/select.js' );

m.module( body, {
  controller,
  view : function(){
    return select( { state : options }, children );
  }
} );

The change looks simple, but might well not be in practice. Some of the advantages of this:

These might sound like dogmatic aesthetic preferences but I believe they could really change the dynamic of mithril.elements. In particular, this would allow 3rd party developers to create plugins with mithril.elements as a dependency, and allow plug-and-play functionality for authors without forcing them to change their code anywhere except where they invoke the plugin itself.

pelonpelon commented 9 years ago

Isn't the transparent composability of user created elements the point of mithril.elements

m( "accordion", {}, [] );

With this change I don't see any advantage over mithril style components.

barneycarroll commented 9 years ago

@pelonpelon you get to keep all the functionality. The only difference is you're referencing the element by direct reference rather than by string to avoid corner-cases. So as well as:

m.element( 'accordion', {} );

m( 'accordion', {}, [] );

You would now (also) be able to do:

var accordion = m.element( {} );

accordion( {}, [] );

And when I say 'uninvasive', this proposal wouldn't introduce any breaking changes in the (documented) mithril.elements API either. It just enables you to be safer. For example, if I want to use the table custom element from the starter kit, I now have to go through all the rest of my code replacing table with $table. And all that code is now mithril.elements-specific and will break in vanilla Mithril.

philtoms commented 9 years ago

I had expected serious element registration to be namespaced. I think perhaps I should mention this by example in the README. I take as precedent the definition of angular directives. I could fully emulate that pattern by allowing multiple implementations bound to the same name, and I did consider this but decided against in preference for speed and simplicity.

@barneycarroll your concerns over escaping element names arise from a misunderstanding. You escape element names from inside a component to prevent recursion. If you have a custom element called select and you want to use the original element type in your custom view, then you need to escape the tag name to prevent Mithril.Elements from calling your custom element again. You would only choose an existing tag name if you want to alter the class behaviour of an element type. If you just want an isolated table-like component with enhanced scrolling, give it a unique name - 'scrollTable'. If you want to change or augment the class of TABLE, then name your component 'table' and escape it internally.

I'm intrigued by the use of partials to register elements, but concerned that the composability that @pelonpelon refers to will be compromised:

m( 'accordion#id.class', {}, [] )

but to be fair, this isn't a major issue for me. I like the clarity of code in the select example - I suspect that this would tally with the ideas of others with regards to reusable components. I don't think that we can have both forms as you indicate without failing to solve the name clash problem that your suggestion proposes to avoid.

I'll look into this and get back to you.

barneycarroll commented 9 years ago

@philtoms your last point is pretty convincing — Mithril's implicit tag name mechanism would screw this unless Elements relegated itself to a distinct API (from my angle, that's still a positive: I prefer unambiguous references, but I can see the argument for seamless adoption). As regards the rest, I should probably start hacking in earnest before crying wolf ;)

pelonpelon commented 9 years ago

I've taken a stab at addressing Barney's issue with the need to refactor vanila mithril.js code in order to wrap it with mithril elements. I've altered these few lines of code:

// mithril.elements.js

  var m = function(module, attrs, children) {
    var tag = module.tag || module;
    var customTag = null;
    if (elements[tag] && tag[0] !== '$') {
      customTag = '.custom-' + tag;
    }
    var args = [customTag || tag].concat([].slice.call(arguments, 1));
    var cell = mithril.apply(null, args);
    if (elements[tag] && tag[0] !== '$') {
      tag = customTag.slice(8);
      cell.tag = tag;
    }
    var element = elements[cell.tag];

mithril.js receives something it expects m( '.custom-table', {}, [] ) and mithril.elements registers m.element('table'). No changes are made to the original mithril.js code. Escaping with '$' is used when the element is registered but never in the original code.

m.element('table') {
    ...
  view: function(ctrl, inner){
    ...
    return m('$table', inner.map(function(row,rowid){
      if (rowid===0){
        return m('thead', {style:{'background-color': '#F66'}}, row);
      }else{
        return m('tbody', row.children.map(function(cell,colid){
          var cellClass = m.prop('cell'+colid+'_'+rowid);
          return m('td', {
            className: cellClass(),
            onclick: ctrl.toggle.bind(ctrl, cellClass)
          }, cell.children);
        }));
      }
    }));

Adding mithril.elements to vanilla mithril.js would be an easy 2 step process.

  1. include mithril.elements.js
  2. include your custom elements (override at will) and you're done! You never touch the original code.

I've created a plunker as a use case.

If you want to have both normal and custom elements of the same type, you might as well just create table and mytable. My edits would be of greater advantage when you want to add functionality to every button in your app, for example.

philtoms commented 9 years ago

@pelonpelon, If you replace mithril-elements-transparent with the original mithril-elements the application behaves the same so I don't see what the name swap is gaining - except an extra unexpected custom-class name.

Barney's issue with $table was down to a small misunderstanding - not by application, he correctly ascertained that escaping will revert to original table behaviour - but by intent: register an element as 'table' to adapt or wrap all table references; register as 'mytable' to target a specific reference.

I have some more considerations on the original proposal:

  1. replacing m with m.e is not technical requirement. The mission statement if you like is to provide custom elements idiomatically, but I am considering the modest api change from m() to me() for those of us who prefer unambiguous APIs. I think it would add confusion though.
  2. the concerns over escaping have been addressed here
  3. the main issue: I believe namespacing is acceptable and even preferable to unique instance identity in this case. It has the advantage of element overrides and augmentation - again discussed in this thread. Would an extra 'already registered' assertion in m.e be beneficial?
pelonpelon commented 9 years ago

@philtoms, I took Barney's momentary misunderstanding as fact and tried to fix something that was working just fine. Very much an un-solution. :P

  1. I support me().
  2. I support namespacing, with only slight concerns about performance in deeply nested elements.
  3. Asserting 'already registered' seems trivial, won't affect any working code, and might prevent collisions, although I can't come up with a plausible example.
  4. I'd like to see direct reference vs string registration. It might allow unrelated elements to communicate. I'm toying with
m.elements = elements    //allow access everywhere within namespace

m( ...
elements.tag.instances[id] = ctrl

m.elements( ...
return elements[root] = { module: ctrl, instances: {} }

// from Todo List
// nextAppointment = elements.calendar.instances[id].getNextAppointment()
philtoms commented 9 years ago

Well I did some poking around and realised that you can separate me() from m() with the following pattern in the main app:

var m = require('mithril') var me = require('mithril.elements')

Try it out if you really want to. By way of experiment I me()'d the whole of the starter kit code - and got a heap of ambiguities that took about an hour with the debugger to sort out. The problem, for me at least, is that me() accepts strings, templates and objects whereas m() only accepts strings. Its not readily apparent that an object passed into component by named reference is a simple string, a template, a component or just a compiled view.

Also, the transparent msx support doesn't work with me() either.

So I will be sticking to the unified API for my own sanity. You may get better milage than me.