Skip to content
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

Performance improvement for templates using <template> #1839

Merged
merged 2 commits into from Aug 8, 2015

Conversation

mbest
Copy link
Member

@mbest mbest commented Aug 6, 2015

For named templates that use <template> or a normal element, use the element directly as the template for cloning nodes. This is instead of parsing the innerHTML.

…element directly as the template for cloning nodes. This is instead of parsing the innerHTML.
@mbest
Copy link
Member Author

mbest commented Aug 6, 2015

This provides an added incentive to use <template> for templates, except in the case of <table> contents since browsers that don't support <template> will choke on those.

@rniemeyer
Copy link
Member

Looks good! I am reviewing this one.

});
});

describe('Anonymous templates', function () {
beforeEach(jasmine.prepareTestNode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't make much real difference, but prepareTestNode is getting called already for each test in the outer beforeEach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've cleaned those up now.

rniemeyer added a commit that referenced this pull request Aug 8, 2015
…eeded

Performance improvement for templates using <template>
@rniemeyer rniemeyer merged commit 7cdbeae into master Aug 8, 2015
@rniemeyer rniemeyer deleted the only-parse-named-template-if-needed branch August 8, 2015 12:46
@rniemeyer rniemeyer mentioned this pull request Aug 30, 2015
10 tasks
@lcorneliussen
Copy link
Contributor

Figured out that putting in in IE11 it just get removed (inner nodes bubble up to the template node)

Anyone seen that before?

@mbest
Copy link
Member Author

mbest commented Jun 11, 2016

@lcorneliussen Can you provide an example?

@IanYates
Copy link

I thought <template> just wasn't supported in IE11 (http://caniuse.com/#search=template).
When @brianmhunt made his new KO documentation page I think the use of <template> was an issue with IE11 (there may also have been the need to add a promise polyfill).

@lcorneliussen
Copy link
Contributor

lcorneliussen commented Jun 11, 2016

[EDIT: I think the code isn't working yet :) - but you get the Idea]

@IanYates Calling document.createElement('template') kindof makes the element 'template' known to IE. But you can't have all element types in it.

$('<template><th><span>x</span></template>').html()yields <span>x</span> in IE (11) - have not tested with older browsers.

Same with div as a wrapper element. So I'm back to cloning each node individually in IE 11 - and not clone at all for IE < 9.

ko.nativeTemplateEngine.prototype['renderTemplateSource'] = function (templateSource, bindingContext, options, templateDocument) {
    var useNodesIfAvailable = !(ko.utils.ieVersion < 9), // IE<9 cloneNode doesn't work properly
        templateNodesFunc = useNodesIfAvailable ? templateSource['nodes'] : null,
        useWrappedNodeIfAvailable = _.isUndefined(ko.utils.ieVersion) && templateNodesFunc && templateNodesFunc.wrapped
        templateNodes = templateNodesFunc ? useWrappedNodeIfAvailable ? templateSource['nodes'].wrapped() : templateSource['nodes']() : null;

    // supporting both array and single-node result from 'nodes'
    if (templateNodes && templateNodes[0] && templateNodes[0].cloneNode) {
        return ko.utils.makeArray(ko.utils.cloneNodes(templateNodes, false))
    }
    else if (templateNodes) {
        return ko.utils.makeArray(templateNodes.cloneNode(true).childNodes);
    } else {
        var templateText = templateSource['text']();
        return ko.utils.parseHtmlFragment(templateText, templateDocument);
    }
};

And in my lazy template source I add nodes.wrapped to the templates:
(this could also somehow get attached to templates automatically)

     var lazyTemplateSource = function (templateId) {
    var self = this;
    self.templateId = templateId;
    self.loaded = false;

    var cached = engine.cache[templateId];

    self.template = ko.observable(cached || "Waiting for template " + templateId + "");
    self.template.nodes = ko.computed({
        deferEvaluation: true,
        read: function () {
            var templateContent = watchTemplateChanges ? self.template() : (cached || self.template());
            var fragment = ko.utils.parseHtmlFragment(templateContent);
            return fragment;
        }
    });
    self.template.nodes.wrapped = ko.computed({
        deferEvaluation: true,
        read: function () {
            var nodes = self.template.nodes();
            var template = document.createElement('template');
            ko.utils.setDomChildren(template, ko.utils.cloneNodes(nodes));
            return template;
        }
    });
    self.template.data = {};
    self.options = {};
    self.options.templateId = templateId;

    if (!cached || watchTemplateChanges) {
        engine.waiting.push(self);
    }
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants