BladeRunnerJS / brjs

BladeRunnerJS (BRJS) is an open source development toolkit and framework for modular construction of large single-page HTML5 apps. It consists of a set of conventions, supporting tools and micro-libraries that make it easy to develop, test, deploy and maintain complex JavaScript apps.
http://bladerunnerjs.org/
GNU Lesser General Public License v3.0
229 stars 36 forks source link

Handling templates when using plain KO. #657

Open soruban opened 10 years ago

soruban commented 10 years ago

Assume you have the following template: `

`

In the js, we would have: var template = ServiceRegistry.getService("br.htmlservice").getHTMLTemplate(this._templateName).cloneNode(true); ko.applyBindings(this, template);

The top level template foo will get loaded fine and display, the bar template however would not be loaded. This is due to the templates being located in an html bundle file and KO not being able to resolve the ID.

Currently to go around the problem, we request the template and append it to the body. Which is not ideal. How do you think this should be handled?

leggetter commented 10 years ago

I don't think I fully understand the problem as I can't see the bar template defined anywhere - but, in the examples apps I've built I use the KnockoutComponent helper class.

e.g.

var KnockoutComponent = require( 'br/knockout/KnockoutComponent' );
var InputViewModel = require( 'brjstodo/todo/input/InputViewModel' );

var App = function() {

  var todoAppEl = document.getElementById( 'todoapp' );

  // todo input Blade
  var inputModel = new InputViewModel();
  var inputComponent = new KnockoutComponent( 'brjstodo.todo.input.view-template', inputModel );
  var inputEl = inputComponent.getElement();
  todoAppEl.appendChild( inputEl );
};

See: https://github.com/BladeRunnerJS/brjs-todomvc-knockout/blob/master/default-aspect/src/brjstodo/App.js

If you can elaborate I'll see if I can replicate the problem and suggest a solution.

Note: Getting the ViewModel and then creating the KnockoutComponent is a bit of unnecessary overhead so I think we may update the KO templates to create a YourClassComponent object which is a `KnockoutComponent so this code would be simplified to the following in the future:

var InputComponent = require( 'brjstodo/todo/input/InputComponent' );

var App = function() {

  var todoAppEl = document.getElementById( 'todoapp' );

  // The InputComponent knows the template ID and the ViewModel to create
  var inputComponent = new InputComponent();
  var inputEl = inputComponent.getElement();
  todoAppEl.appendChild( inputEl );
};
dchambers commented 10 years ago

I just spoke with @soruban, and it seems that the problem is related to the 'ko' library, rather than the 'presenter-knockout' library.

Although the heavilly patched 'presenter-knockout' library has been patched to use the br.ServiceRegistry.getService('br.html-service') to retrieve templates, the pure 'ko' library hasn't been patched al all, and so doesn't allow bundled sub-templates to be used.

Although it's possible to place all HTML templates directly into the index page, this goes completely against the BRJS philosophy of creating business features in blades that can easily be added and removed from an application.

andy-berry-dev commented 10 years ago

@leggetter has already managed to make this work (see above comment). We shouldn't change the 'ko' library, it's there to be the used as the vanilla knockout.

leggetter commented 10 years ago

I don't think I've tried to do sub-templates in the way @soruban has described - where KO needs to know how to retrieve the bar template from the br.htmlservice.

We definitely shouldn't hack the KO library. However, KO seems much more extensible that it originally was. For example, here's how you can use the underscore templating engine: http://jsfiddle.net/rniemeyer/NW5Vn/

However, I don't think this helps with getting the template object - which is the problem.

Hopefully there will be a way of extending KO to allow the call to ServiceRegistry.getService("br.htmlservice").getHTMLTemplate(templateId) to be used for sub-templates.

soruban commented 10 years ago

Hey, just discussed with @dchambers, for now we will use a custom KO with the line to allow resolving subtemplates via the service. I do think that you do need a version of KO in brjs that supports this out of the box or you would not be able to use brjs correctly.

leggetter commented 10 years ago

Looks like you could create a template engine based on the default one and just override the makeTemplateSource implementation: https://github.com/BladeRunnerJS/brjs/blob/master/cutlass-sdk/workspace/sdk/libs/javascript/thirdparty/presenter-knockout/brjs-knockout-3.0.js#L3406

Not sure about the best way of making this custom/extended templating engine the default though. I'd suggested we keep this issue open and we feed it into future planning.

dchambers commented 10 years ago

@leggetter, so I guess it sounds like you'd prefer a 'brjs-ko' library, that depends on the 'ko' library but patches it from the outside to make it compatible with BRJS. I haven't tested it, but it looks like it should be possible to do this by writing some code like this:

ko.templateEngine.prototype._makeTemplateSource = ko.templateEngine.prototype.makeTemplateSource;

ko.templateEngine.prototype.makeTemplateSource = function(template, templateDocument) {
    var fakeDocument = {
        getElementById: function(id) {
            var elem = templateDocument.getElementById(id);

            if(!elem) {
                elem = br.ServiceRegistry.getService('br.html-service').getHTMLTemplate(id);
            }

            return elem;
        }
    };

    ko.templateEngine.prototype._makeTemplateSource.call(this, template, fakeDocument);
};
james-shaw-turner commented 10 years ago

Definitely preferable to patch from the outside with a standard extension mechanism

leggetter commented 10 years ago

That'd work. But overriding the default templating engine seems a bit 'naughty' :)

Maybe something like:

var br = require( 'br/Core' );
var ko = require( 'ko' );
var ServiceRegistry = require( 'br/ServiceRegistry' );

function BRJSKoTemplateEngine = function() {
  this._htmlService = ServiceRegistry.getService( 'br.html-service' );
};
// Not sure this will work - depends on how KO objects are defined
br.extend( BRJSKoTemplateEngine, ko.underscoreTemplateEngine );

// Override super definition
BRJSKoTemplateEngine.prototype.makeTemplateSource = function(template, templateDocument) {
  // Named template
  if (typeof template == "string") {
    templateDocument = templateDocument || document;
    // Only change to the default implementation is here.
    var elem = this._htmlService.getHTMLTemplate( template );
    if (!elem) {
      throw new Error("Cannot find template with ID " + template);
    }
    return new ko.templateSources.domElement(elem);
  }
  else if ((template.nodeType == 1) || (template.nodeType == 8)) {
    // Anonymous template
    return new ko.templateSources.anonymousTemplate(template);
  }
  else {
    throw new Error("Unknown template type: " + template);
  }
};

// Not sure where this bit should be called
ko.setTemplateEngine( new BRJSKoTemplateEngine() );

This seems cleaner, but does beg the questions:

  1. where should ko.setTemplateEngine( new BRJSKoTemplateEngine() ); be called?
  2. Would anybody ever expect the normal KO makeTemplateSource functionality so is @dchambers solution a simpler one?
soruban commented 10 years ago
  1. Would anybody ever expect the normal KO makeTemplateSource functionality

Ideally, both should work: you first check if the template is defined in the current HTML, if not, use the resource-service to look into the bundle.html.

dchambers commented 10 years ago

I'd say so. In my solution both approaches work, whereas in @leggetter's solutions only loading templates from the HTML resource service is possible (though this would be very easy to fix).